boot-clj / boot-cljs

Boot task to compile ClojureScript programs.
Eclipse Public License 1.0
176 stars 40 forks source link

.cljs.edn not reloaded? #108

Closed magomimmo closed 9 years ago

magomimmo commented 9 years ago

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

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

magomimmo commented 9 years ago

(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 https://github.com/adzerk-oss/boot-cljs/issues/108#issuecomment-154344410.

martinklepsch commented 9 years ago

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

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

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

magomimmo commented 9 years ago

take your time! and thanks again.

magomimmo commented 9 years ago

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

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

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

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 https://github.com/adzerk-oss/boot-cljs/issues/108#issuecomment-154695702.

magomimmo commented 9 years ago

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 https://github.com/adzerk-oss/boot-cljs/issues/108#issuecomment-154695769.

Deraen commented 9 years ago

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

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

Deraen commented 9 years ago

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

magomimmo commented 9 years ago

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 https://github.com/adzerk-oss/boot-cljs/commit/5a614c7ef8df1cd7f809a9204cac4eb49f329013 — Reply to this email directly or view it on GitHub https://github.com/adzerk-oss/boot-cljs/issues/108#issuecomment-154697293.

magomimmo commented 9 years ago

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

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

Deraen commented 9 years ago

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.