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

.cljs.edn not reloaded? #108

Closed
magomimmo opened this issue Nov 6, 2015 · 18 comments
Closed

.cljs.edn not reloaded? #108

magomimmo opened this issue Nov 6, 2015 · 18 comments

Comments

@magomimmo
Copy link

Hi, when I use *.cljs.edn to change the default output-to and asset-path compiler options, if I'm working in a kind of Immediate Feedback Development Environment (i.e. boot-http + watch + boot-reload + boot-cljs-repl + boot-cljs) and create a new cljs file/namespace, even if I add the new namespace to the corresponding :require section of the above edn file, to see the effects I have to stop boot and relaunch it, which is annoying.

Is there a trick I don't know about, or it's a known feature/issue?

Thanks

@martinklepsch
Copy link
Member

@magomimmo can you paste your task definition? I think task order is most likely the source of this problem :)

@magomimmo
Copy link
Author

(deftask dev
"Launch immediate feedback dev environment"
[](comp
%28serve :dir)
(watch)
(reload)
(cljs-repl) ;; before cljs
(cljs)))

thanks!
mimmo

On 06 Nov 2015, at 09:35, Martin Klepsch notifications@github.com wrote:

@magomimmo https://github.com/magomimmo can you paste your task definition? I think task order is most likely the source of this problem :)


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

@martinklepsch
Copy link
Member

Ok that seems right. One thing to keep in mind is that changing output-to is handled via the location of your cljs edn file and not via an explicit option.

Have you looked at the generated shim? (The file you load in your html page)
Does it not change at all when you edit your cljs.edn requires?

@magomimmo
Copy link
Author

whan I save the cljs.edn requires. I see the reload notification in the console of the developer tool (chrome) which notifies me about /js/main.cljs.edn been reloaded. I also see the cljs recompilation been triggered in the terminal from where I started boot dev.

But if I look into the js/main.out/ directory the new namespace is not there and neither in source panel of the chrome developer tool.

if could be helpful, you can checkout the following repo:

https://github.com/magomimmo/modern-cljs/tree/se-tutorial-04

if you launch the boot dev command, everything works as expected. then stop boot process and do the following:

delete the following files

html/login.html
src/cljs/modern_cljs/login.cljs

remove modern-cljs.login from the requires section of the /js/main.cljs.edn file.

relaunch the boot dev command then:

  1. readd the html/login.html file (i.e. git checkout -- html/login.html)
  2. readd the src/cljs/modern_cljs/login.cljs file (i.e. git checkout -- src/cljs/modern_cljs/login.cljs)
  3. edit the js/main.cljs.edn and readd the modern-cljs.login namespace in the require section.

Even if everything get reloade, the modern_cljs.login namespace is not available.

HIH to better understand the behaviour.

Thanks so much for your help.

@martinklepsch
Copy link
Member

Thanks for the update, I'll take a look some time later today, deep in the middle of some other stuff right now :)

@magomimmo
Copy link
Author

take your time! and thanks again.

@magomimmo
Copy link
Author

Hi Martin, the problem persists even if I do not use the .cljs.edn file to overwrite :output-to and asset-path compiler options.

When I add new cljs files/namespaces I have to stop boot/JVM to see the new cljs/namespaces in the target directory and in the sources panel of the Development Tools as well.

You can test it with this minimal repo:

https://github.com/magomimmo/modern-cljs/tree/boot-cljs-testing

You can test by yourself doing the following:

git clone https://github.com/magomimmo/modern-cljs.git
cd modern-cljs
git checkout boot-cljs-testing
boot dev

then visit http://localhost:3000/login.html and see in the sources' panel of the developer tool that you have both the modern-cljs.core and modern-cljs.login cljs/js source files.

Then you could procede as follows after having stopped the boot process:

rm html/css/styles.css
rm html/login.html
rm src/cljs/modern_cljs/login.cljs
boot dev

Finally, you could start readding the deleted file to the project while the boot dev process is running:

# in a second terminal
cd /path/to/modern-cljs
git checkout -- html/css/styles.css
git checkout -- html/login.html
git checkout -- src/cljs/modern_cljs/login.cljs

to verify that no modern-cljs.login file/namespace has been added to the sources' panel of the development tool of your browser (I use chrome canary).

Thanks again for your support. Probably I'm doing something very stupid somewhere....

@Deraen
Copy link
Member

Deraen commented Nov 7, 2015

The problem with the example is not related to compiler options, but probably due to a bug with Boot-cljs logic about writing main namespace shim only when cljs.edn file has been changed: https://github.com/adzerk-oss/boot-cljs/blob/master/src/adzerk/boot_cljs.clj#L207-L212

This probably also causes the problems with adding require entries to manually created cljs.edn.

:compiler-options

Back to compiler options, Boot-cljs doesn't have any logic for resetting compiler environment when build affecting options change and we probably wont have. This should be handled by ClojureScript compiler itself: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/compiler.cljc#L1278-L1281 but it's possible that this is not very reliable.

To check if problem is in ClojureScript compiler and not on Boot-cljs, you should test this without any build tools: https://github.com/clojure/clojurescript/wiki/Reporting-Issues

@Deraen
Copy link
Member

Deraen commented Nov 7, 2015

TLDR; I don't recommend changing :asset-path or other compiler-options. Adding :requires should work and if it doesn't, it's a bug in boot-cljs.

@magomimmo
Copy link
Author

thanks so much. I’m in the process to migrate the entire modern-cljs series of tutorials from leiningen to boot, which I like a lot becasue I can use a single JVM to setup a complete immediate feedback developer environment (using cider as well).

I’ll workaround this problem and check if the problem is in the cljs compiler.

thanks again

On 07 Nov 2015, at 13:17, Juho Teperi notifications@github.com wrote:

The problem with the example is not related to compiler options, but probably due to a bug with Boot-cljs logic about writing main namespace shim only when cljs.edn file has been changed: https://github.com/adzerk-oss/boot-cljs/blob/master/src/adzerk/boot_cljs.clj#L207-L212 https://github.com/adzerk-oss/boot-cljs/blob/master/src/adzerk/boot_cljs.clj#L207-L212
This probably also causes the problems with adding require entries to manually created cljs.edn.

:compiler-options

Back to compiler options, Boot-cljs doesn't have any logic for resetting compiler environment when build affecting options change and we probably wont have. This should be handled by ClojureScript compiler itself: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/compiler.cljc#L1278-L1281 https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/compiler.cljc#L1278-L1281 but it's possible that this is not very reliable.

To check if problem is in ClojureScript compiler and not on Boot-cljs, you should test this without any build tools: https://github.com/clojure/clojurescript/wiki/Reporting-Issues https://github.com/clojure/clojurescript/wiki/Reporting-Issues

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

@magomimmo
Copy link
Author

I’ll check. Thanks

On 07 Nov 2015, at 13:18, Juho Teperi notifications@github.com wrote:

TLDR; I don't recommend changing :asset-path or other compiler-options. Adding :requires should work and if it doesn't, it's a bug in boot-cljs.


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

@Deraen
Copy link
Member

Deraen commented Nov 7, 2015

Did a bit more debugging. The cljs.edn change logic is okay. But cljs.edn is read only once: https://github.com/adzerk-oss/boot-cljs/blob/master/src/adzerk/boot_cljs.clj#L100-L114

@Deraen
Copy link
Member

Deraen commented Nov 7, 2015

@magomimmo Could you try with latest master? I think this should fix this both with automatically generated cljs.edn and manually created: 5a614c7

@Deraen
Copy link
Member

Deraen commented Nov 7, 2015

Also, now that cljs.edn is correctly read each time, it is possible that changing compiler-options works.

@magomimmo
Copy link
Author

Juho, I’m going to a funeral of a friend right now :-(. I’ll check the master tomorrow morning…
thanks so much

mimmo

On 07 Nov 2015, at 13:39, Juho Teperi notifications@github.com wrote:

@magomimmo https://github.com/magomimmo Could you try with latest master? I think this should fix this both with automatically generated cljs.edn and manually created: 5a614c7 5a614c7

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

@magomimmo
Copy link
Author

Hi Juho, I checked the master and as soon as I copy a file, even a css file, while the boot process is active, I now receive the following warning in the browser console:

Failed to execute 'write' on 'Document': It isn't possible to write into a document from an asynchronously-loaded external script unless it is explicitly opened.

It seem to have to do with the following known problem:

http://stackoverflow.com/questions/24297829/execute-write-on-doc-it-isnt-possible-to-write-into-a-document-from-an-asynchr

Thanks for your help

@magomimmo
Copy link
Author

The above said, now the main.cljs.edn file is correctly reload and the new namespace it requires as well.

@Deraen
Copy link
Member

Deraen commented Nov 8, 2015

Okay, now boot-cljs seems to work correctly but it has revealed a bug in boot-reload. It's because boot-reload will try to reload non-reloadable namespaces, such as the shim ns. For now you'll just need to do full reload when changing :reload on cljs.edn.

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

3 participants