emezeske / lein-cljsbuild

Leiningen plugin to make ClojureScript development easy.
Other
1.1k stars 152 forks source link

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

Closed cemerick closed 7 years ago

cemerick commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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