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

fix reloading of clj files in cljs dirs #349

Merged
merged 1 commit into from
Dec 15, 2014
Merged

fix reloading of clj files in cljs dirs #349

merged 1 commit into from
Dec 15, 2014

Conversation

bhauman
Copy link
Contributor

@bhauman bhauman commented Dec 12, 2014

cljsbuild “auto” produces different results than “once” if a clj file that is a dependency of a clj based macro file changes. This is because non-macro clj files are not reloaded on change.

This issue #210 is why a guard was put in to prevent the loading of non macro clj files. This issue appears to have been resolved as of clojurescript 2202.

And so I removed the guard that only allows .clj files that contain macros to be reloaded. I have tested this and it works pretty darn good :)

cljsbuild “auto” produces different results than “once” if a clj file
that is a dependency of a clj based macro file.  This is because
non-macro clj files are not reloaded on change.

The issue behind #210
appears to have been resolved as of clojurescript 2202.

I removed the guard that only allows .clj files that contain macros to
be reloaded.  I have tested this and it works pretty darn good :)
@bhauman
Copy link
Contributor Author

bhauman commented Dec 15, 2014

Actually I think this behavior may justify a flag in build options.
:reload-non-macro-clj-source true
Maybe it should default to false.

@cemerick
Copy link
Collaborator

If the underlying issue is resolved >= 2202, why have a flag?

@bhauman
Copy link
Contributor Author

bhauman commented Dec 15, 2014

I talked to David and couldn't find the underlying issue. The warnings for your test case went away as of 2202. Don't know enough to guarantee that it won't cause problems ever. You seem more familiar with the cause. Is there reason for concern?

@cemerick
Copy link
Collaborator

I remember now that the real fix was tools.reader using its own facility (a dynamically-bound map, I think) representing ClojureScript namespaces and their aliases, instead of abusing Clojure namespaces. I'll verify that this change doesn't cause a regression with the minimal case I put together for #210 and merge.

@bhauman
Copy link
Contributor Author

bhauman commented Dec 15, 2014

I ran the foo.clj test about a dozen times to find the release where it
broke :)

Thanks for looking at this.

On Mon, Dec 15, 2014 at 1:43 PM, Chas Emerick notifications@github.com
wrote:

I remember now that the real fix was tools.reader using its own facility
(a dynamically-bound map, I think) representing ClojureScript namespaces
and their aliases, instead of abusing Clojure namespaces. I'll verify that
this change doesn't cause a regression with the minimal case I put together
for #210 #210 and
merge.


Reply to this email directly or view it on GitHub
#349 (comment)
.

@cemerick cemerick merged commit 5485e25 into emezeske:master Dec 15, 2014
@cemerick
Copy link
Collaborator

@bhauman then why would I test it more? ;-)

Thanks!

@cemerick cemerick added this to the 1.0.4 milestone Dec 15, 2014
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.

2 participants