Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: send custom event with code eval state payload #296

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

KBHan
Copy link

@KBHan KBHan commented Dec 3, 2017

@KBHan KBHan force-pushed the feat-eval-dispatch-custom-event branch from 10af53f to 3b0fa4d Compare December 3, 2017 22:35
@dviramontes
Copy link

Note: this is still a bit of a WIP / need to find a better way to get a hold of the klipse container selector

@@ -52,7 +52,8 @@
(swap! state update-in [:eval-counter] inc)
(let [evaluation-chan (eval-fn (str preamble src-code) @state)
first-result (<! evaluation-chan)]
(setter first-result)
(setter first-result)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you can handle different cases for first-result:

  1. if it is a string, behave as before
  2. it it is an array, it will be either: [:ok result] or [:err result]

In python.cljs, returns either [:ok result] or [:err result].

Dispatch the event from here and not from python.cljs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @viebel, looks like we are only seeing case # 1:

if it is a string, behave as before

and never case # 2
do you have any idea why ? We are evaluating python code.

Copy link
Owner

@viebel viebel Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dviramontes you have to modify the code in python.cljs so that it returns

  • [:ok result] - in case of success
  • [:err error] - in case of error

In order not to break other languages, you have to handle also cases where a string was returned.
Does it make sense?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viebel yes it does, i'm going to give it a shot and let you know how it goes. Thanks!

@dviramontes
Copy link

@viebel I updated the PR. Lmk if this is closer to what you had proposed. Thanks!

@dviramontes
Copy link

dviramontes commented Dec 30, 2017

Also closes #292

(fn [err]
(put! c (str "error: " err)))))
(put! c (str "error: " err))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of an error, you sould put only [:err err] on the channel

(setter results)
(if (string? result)
(setter results)
(when-let [klipse-dom-node (js/document.querySelector ".klipse-container")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case result is not a string you should call (setter (second results))

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you recommend I do both dispatch custom event and call (setter (second results)) here ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dviramontes yes I recommend I do both dispatch custom event and call (setter (second results)) here

(let [event-payload (clj->js {:detail {:result result
:result-element (:result-element @state)}})]
(!> klipse-dom-node.dispatchEvent
(js/CustomEvent. "klipse-snippet-evaled" event-payload)))))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add in the detail of the event whether there was an error or not

@dviramontes
Copy link

@viebel made some changes, lmk what you think / thanks!

@@ -142,14 +155,9 @@
(defmethod create-editor :code-mirror [_ {:keys [snippet-num element source-code eval-fn default-txt idle-msec editor-in-mode editor-out-mode indent? codemirror-options-in codemirror-options-out loop-msec preamble no-result] :as editor-args}]
(let [[in-editor-options out-editor-options] (editor-options editor-in-mode editor-out-mode codemirror-options-in codemirror-options-out)
container (create-div-after element (klipse-container-attrs snippet-num))
_ (create-div-after container {:class "klipse-separator"})
Copy link
Owner

@viebel viebel Jan 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bring back the line that creates the klipse-separator

:klass "klipse-result")) ; must be called before `element` is replaced

in-editor (replace-element-by-editor element source-code in-editor-options :indent? indent? :klass "klipse-snippet")
result-element (when-not no-result (create-editor-after-element element default-txt out-editor-options :indent? false :remove-ending-comments? false)) ; must be called before `element` is replaced
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break this line in several lines as it was before

@dviramontes dviramontes force-pushed the feat-eval-dispatch-custom-event branch from d95b30d to 5dd7971 Compare February 8, 2018 04:40
@dviramontes
Copy link

dviramontes commented Feb 8, 2018

@viebel updated PR / sorry I must have rebased it against my fork therefore the changes to klipse editor. Should be good now. Thanks!

(if (string? result)
(setter results)
(let [hasError (and (= (first result) :err))]
(when hasError (setter (second result))) ;; only report the :err messages for now
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to call setter even when there is no error.
The reason is that when the code evaluation succeeds, we pass [:ok res]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise everything looks fine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @viebel the problem with setting (setter (second result)) here when there is no error is that the receiver throws
screen shot 2018-02-15 at 7 46 06 pm

I think is is because the message passed in from
klipse/lang/python.cljs:33

(put! c [:ok mod])

puts something on that channel that cannot be stringified.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if that makes sense.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have to call str before putting on the channel.

(put! c [:ok (str mod)])

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense but let me show you what im running into now, it might make more sense to you..
feb-15-2018 23-18-01

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^That's with (put! c [:ok (str mod)]). So its not longer throwing an error but we no longer get the output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants