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

Clojure code is reloaded blindly, breaks protocol+multimethod impls #455

Closed
cemerick opened this issue Nov 16, 2016 · 4 comments
Closed

Comments

@cemerick
Copy link
Collaborator

cljsbuild reloads changed Clojure namespaces without regard for the dependencies among them. If namespaces are loaded in the wrong order, then various bad things can happen, including:

  • types implementing protocols will not be recognized as doing so if the protocol's namespace is reloaded after those containing the types
  • reloading namespaces containing multimethod definitions drops any multimethod implementations loaded previously

Other ramifications are discussed here.

(In hindsight, I'm surprised that this hasn't been reported as an issue before [AFAICT]. The order of reloading is entirely based on file enumeration order [which varies on different operating/file systems], so provoking the problem isn't hard.)

A minimal example of this problem is available here: https://github.com/cemerick/cljsbuild-reload-bug-repro

If you attempt to build that project as it sits, you'll get:

$ lein do clean, cljsbuild once
Compiling ClojureScript...
loading foo.b
loading foo.a
loading foo.b
loading foo.c
Reloading Clojure file "/foo/c.clj" failed.
clojure.lang.Compiler$CompilerException: java.lang.IllegalArgumentException: No implementation of method: :method of protocol: #'foo.b/Protocol found for class: foo.a.Thing, compiling:(foo/c.clj:6:1)

foo.a contains a defrecord type that implements a protocol in foo.b. Since cljsbuild (on my operating/file system, anyway) loads foo.b after foo.a, the type is determined to not implement the (now changed) protocol, and the top-level usage in foo.c fails.

The "workaround" I can think of in this scenario is to wrap the foo.b protocol definition in a defonce, thus preventing it from being changed after being first required by foo.a (this change is included in the source of foo.b, commented out):

$ lein do clean, cljsbuild once
Compiling ClojureScript...
loading foo.b
loading foo.a
loading foo.b
loading foo.c
:result
Compiling "target/foo.js" from ["src" "test"]...
Successfully compiled "target/foo.js" in 0.426 seconds.

The only real solution available AFAICT would be to use tools.namespace to manage the reloading. I wouldn't expect this to be difficult, but I have no idea how the change might subtly change existing behaviour, interact with the ClojureScript compiler, etc.

@chadharrington
Copy link

This issue is affecting us in production code; it would be great to have a fix.

Thanks for your efforts in making lein-cljsbuild a great tool.

@mneise
Copy link
Collaborator

mneise commented Dec 16, 2016

@cemerick Thank you for the detailed issue description and the repo to reproduce the problem! Very appreciated 😃

I have a first version working using clojure.tools.namespace and deployed it as 1.1.6-SNAPSHOT. It seems to fix the problem for the supplied repo, but I'm still making improvements. If you encounter any further issues using the SNAPSHOT let me know ;)

@danielcompton
Copy link
Contributor

danielcompton commented Dec 16, 2016

It might be a good idea to shade clojure.tools.namespace as Leiningen bundles its own version and this can lead to various hard to diagnose bugs. Perhaps using https://github.com/benedekfazekas/mranderson?

Edit: I think I misread and Leiningen doesn't use it. Nevertheless it is probably a good idea to shade it so that other plugins don't interfere with your dependencies.

@chadharrington
Copy link

chadharrington commented Dec 19, 2016

@mneise thanks so much for your fix! Your 1.1.6-SNAPSHOT appears to have resolved our issue.

@mneise mneise closed this as completed in 7a28aaf Apr 27, 2017
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

No branches or pull requests

4 participants