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

bump suitable to fix #643 #644

Merged
merged 1 commit into from
Sep 2, 2019
Merged

bump suitable to fix #643 #644

merged 1 commit into from
Sep 2, 2019

Conversation

rksm
Copy link
Member

@rksm rksm commented Sep 1, 2019

  • nodejs would throw nullpointer exception on first completion due to repl-env
    not being initialized

Before submitting a PR make sure the following things have been done:

  • [yep] The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • [yep] All tests are passing
  • [yep] The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

Note: If you're just starting out to hack on cider-nrepl you might find
nREPL's documentation and the
"Design" section of the README extremely useful.*

Thanks!

@rksm
Copy link
Member Author

rksm commented Sep 1, 2019

waiting for clojars to be up again...

@bbatsov
Copy link
Member

bbatsov commented Sep 1, 2019

Same here. :-)

@rksm
Copy link
Member Author

rksm commented Sep 1, 2019

Looking into the test errors...

@dpsutton
Copy link
Contributor

dpsutton commented Sep 1, 2019

I'm running the tests locally but its not related to you. :symbol was renamed to :sym and the integration tests in info_spec_test.clj need to be updated accordingly.

@rksm
Copy link
Member Author

rksm commented Sep 1, 2019

Hmm I believe there is also an issue with my fix. Apparently calling cljs.repl/setup during the completion call with a nashorn/repl-env won't produce completions somehow... looking into it.

@dpsutton
Copy link
Contributor

dpsutton commented Sep 1, 2019

I was incorrect. The tests fail due to upstream in orchard.meta

(defn var-meta
  "Return a map of metadata for var v.
  If whitelist is missing use var-meta-whitelist."
  ([v] (var-meta v var-meta-whitelist))
  ([v whitelist]
   (when (var? v)
     (let [meta-map (-> (meta v)
                        maybe-protocol
                        (select-keys (or whitelist var-meta-whitelist))
                        map-seq
                        maybe-add-file
                        maybe-add-url
                        (update :ns ns-name))]
       (maybe-add-spec v meta-map) ;; this spec can never be returned
       (maybe-add-see-also v meta-map)))))

@bbatsov
Copy link
Member

bbatsov commented Sep 1, 2019

@dpsutton Hmm, that's weird. I think the relevant tests were passing for me last time I checked them. I'll look into this after Clojure/south.

@bbatsov
Copy link
Member

bbatsov commented Sep 1, 2019

Hmm I believe there is also an issue with my fix. Apparently calling cljs.repl/setup during the completion call with a nashorn/repl-env won't produce completions somehow... looking into it.

Got it. Let me know when you manage to track this down.

@rksm
Copy link
Member Author

rksm commented Sep 1, 2019

So I found the root cause but don't know how to fix it yet.

cljs.repl.node uses a hand-made (not via a dynamic var but the thread name) thread-local buffer to store evaluation results (https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl/node.clj#L127). The thread that completions are computed in does not seem to be the same as the original thread for creating the repl env (names are something like clojure-agent-send-off-pool-11 for completion and nRepl-session-08c91d1c-7c45-4e0b-9ffd-76f6588cc4cc for initialization).

So when suitable tries to eval we hit a hard nullpointer exception. I could try to add custom code for node here but this seems just a hack on a hack. Any idea what best to do here?

@bbatsov
Copy link
Member

bbatsov commented Sep 2, 2019

Well, the way I see we either add custom code or we ignore node. I'm not the world's greatest expert in ClojureScript, though. :D

- nodejs would throw nullpointer exception on first completion due to repl-env
not being initialized
@rksm
Copy link
Member Author

rksm commented Sep 2, 2019

The issue should be fixed now.

@bbatsov bbatsov merged commit 1150a3d into clojure-emacs:master Sep 2, 2019
@bbatsov
Copy link
Member

bbatsov commented Sep 2, 2019

Thanks! I'll cut a new release later today.

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.

3 participants