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

cljc buffers not evaluated in cljs repl by cider-load-buffer #1875

Closed
leppert opened this issue Oct 28, 2016 · 11 comments
Closed

cljc buffers not evaluated in cljs repl by cider-load-buffer #1875

leppert opened this issue Oct 28, 2016 · 11 comments
Labels

Comments

@leppert
Copy link

leppert commented Oct 28, 2016

Expected behavior

After running cider-jack-in-clojurescript, cider-load-buffer loads the cljc buffer in both the Clojure and ClojureScript REPLs per the cider-load-buffer docstring.

Actual behavior

The buffer is evaluated in the Clojure REPL but not the ClojureScript REPL.

Steps to reproduce the problem

  1. Check out this sample project: https://github.com/leppert/cljc-bug
  2. Open src/cljc_bug/core.cljc
  3. Run cider-jack-in-clojurescript
  4. C-c M-n to load the namespace. Note: this works correctly in both REPLs
  5. C-c C-k to eval the buffer
  6. Calling (foo "bar") in the Clojure REPL will now correctly print bar Hello, World!
  7. Calling (foo "bar") in the ClojureScript REPL will throw the error WARNING: Use of undeclared Var /foo at line 1 <cljs repl>

image

Environment & Version information

CIDER version information

;; CIDER 0.15.0snapshot (package: 20161016.424), nREPL 0.2.12
;; Clojure 1.8.0, Java 1.8.0_40

Lein/Boot version

Leiningen 2.7.1 on Java 1.8.0_40 Java HotSpot(TM) 64-Bit Server VM

Emacs version

GNU Emacs 25.1.1 (x86_64-apple-darwin16.0.0, NS appkit-1504.00 Version 10.12 (Build 16A320)) of 2016-09-21

Operating system

macOS 10.12 (16A320)

@bbatsov bbatsov added the bug label Oct 30, 2016
@bbatsov
Copy link
Member

bbatsov commented Oct 30, 2016

Sounds like some regression to me. Probably fixing this would be easy, but I won't be able to look into this for a while.

@leppert
Copy link
Author

leppert commented Oct 30, 2016

@bbatsov I'll give it a stab.

@leppert
Copy link
Author

leppert commented Oct 30, 2016

I've confirmed that simply changing the major mode of the .cljc buffer from clojurec-mode to clojurescript-mode leads to the buffer being loaded in the ClojureScript REPL correctly. To narrow this down further, it appears that:

@leppert
Copy link
Author

leppert commented Oct 30, 2016

Still digging, but it looks like after the load-file lambda is called on other-connection, (cider-current-connection "cljs") starts to return nil because cider--connection-properties reports the cljs REPL connection as :type "clj".

It looks like something is setting the ClojureScript REPL cider-repl-type to "clj".

@leppert
Copy link
Author

leppert commented Oct 30, 2016

Beginning to wonder if this has something to do with the nrepl callback queue. Here's a log of activity when cider-load-buffer is called:

cider-request:load-file *cider-repl cljc-bug*
cider-nrepl-send-request *cider-repl cljc-bug*
nrepl-send-request connection=*cider-repl cljc-bug* id=27, nrepl-session=1c5efd99-9886-4dab-9277-8ced6fc448e0
cider-request:load-file *cider-repl CLJS cljc-bug*
cider-nrepl-send-request *cider-repl CLJS cljc-bug*
nrepl-send-request connection=*cider-repl CLJS cljc-bug* id=27, nrepl-session=a1e9394e-766e-45cb-8c89-c6455c47d906
Loading /Users/leppert/github/cljc-bug/src/cljc_bug/core.cljc...
cider-repl--state-handler response=(dict id 27 session 1c5efd99-9886-4dab-9277-8ced6fc448e0 value #’cljc-bug.core/foo)
=> #'cljc-bug.core/foo
cider-repl--state-handler response=(dict id 27 session 1c5efd99-9886-4dab-9277-8ced6fc448e0 status (done))
cider-repl--state-handler response=(dict changed-namespaces (dict) id 27 repl-type clj session 1c5efd99-9886-4dab-9277-8ced6fc448e0 status (state))
cider-repl--state-handler response=(dict id 27 session 1c5efd99-9886-4dab-9277-8ced6fc448e0 value #’cljc-bug.core/foo)
=> #'cljc-bug.core/foo
cider-repl--state-handler response=(dict id 27 session 1c5efd99-9886-4dab-9277-8ced6fc448e0 status (done))
cider-repl--state-handler response=(dict changed-namespaces (dict) id 27 repl-type clj session 1c5efd99-9886-4dab-9277-8ced6fc448e0 status (state))

With the caveat that I know very little about this system, it looks like the callbacks are called twice for the clj connection (session id 1c5efd99-9886-4dab-9277-8ced6fc448e0) rather than once for the clj connection and once for the cljs connection (session id a1e9394e-766e-45cb-8c89-c6455c47d906).

@Malabarba
Copy link
Member

Perhaps it's possible that the state tracker middleware is below the load-file middleware, so once you call load-file cider gets confused. Technically I can't see how that would produce the results you observe, but it's the closest I can think of to a lead. 😕

@dpsutton
Copy link
Contributor

dpsutton commented Dec 2, 2016

So the code goes through cider-map-connections twice. The first time it knows it is supposed to work on :both buffers. And current-buffer is the source file so it allows it. However, point moves to the repl and so when it goes through this call again, current-buffer is no longer a buffer deriving from clojurec-mode or clojurex-mode, and therefore it drops :both for :any.

In setting the which binding to :any, :cljs :clj or :both, if its both, it makes sure its in a buffer than :both makes sense, otherwise it falls back to any, which seems to send it to the first connection that puts its hand up.

@Malabarba
Copy link
Member

OK. It definitely shouldn't go through the function the second time. I guess we need to rethink the code flow here a bit.

@dpsutton
Copy link
Contributor

Ok, I think I've gotten to the bottom of the issue. The problem is that the session for cljs is being marked as clj type, as @leppert mentioned earlier. The issue (I think) is that when you load the buffer, it doesn't fully respect the connection that its given:

(defun cider-request:load-file (file-contents file-path file-name &optional connection callback)
  "Perform the nREPL \"load-file\" op.
FILE-CONTENTS, FILE-PATH and FILE-NAME are details of the file to be
loaded.

If CONNECTION is nil, use `cider-current-connection'.
If CALLBACK is nil, use `cider-load-file-handler'."
  (cider-nrepl-send-request (list "op" "load-file"
                                  "session" (cider-current-session)
                                  "file" file-contents
                                  "file-path" file-path
                                  "file-name" file-name)
                            (or callback
                                (cider-load-file-handler (current-buffer)))
                            connection))

So here, the connection is looked up but session is grabbed independently of this. And there's our bug. We say do this to cljs repl and it does on that connection, but it grabs the clj session info. And when the state info comes back, it marks it as a clj repl for the future.

@dpsutton
Copy link
Contributor

This can be closed now as well.

@eggsyntax
Copy link

Great work on this, y'all, this removes a major pain point for me. Really appreciate this, and all of your work :) 💯

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

No branches or pull requests

6 participants