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

Default main.cljs.edn generation should be customizable from other tasks #129

Open
arichiardi opened this issue May 29, 2016 · 10 comments
Open

Comments

@arichiardi
Copy link
Contributor

arichiardi commented May 29, 2016

The need came out from a boot-cljs-devtools issue.

The scenario: the user does not specify cljs.edn files explicitly. In general a task before cljs might need to add a :require or :init-fns clause to the default main.cljs.edn generated by cljs.

At the moment there is no way to do that and cljs-devtools blindly ignores the fact that we have no .cljs.edn skipping adding stuff to it. Of course, changing the order works (e.g.: (boot ... (cljs) (cljs-devtools), but in general my idea is to have ways to add stuff to :require (or :init-fns or ...) from tasks before cljs.

The idiomatic way is to add metadata to files in the fileset but I wanted to ask here what we should mark and of course if you folks consider this a good idea or total garbage 😄

Thanks!

@Deraen
Copy link
Contributor

Deraen commented May 29, 2016

Idiomatic solution is that the user creates the .cljs.edn file when using boot-cljs-devtools or other tasks that modify the file.

I don't think having the options in another place (metadata) in addition to the file is a good idea.

@arichiardi
Copy link
Contributor Author

isn't that optional? I have a couple of projects without the file and they work fine...

@arichiardi
Copy link
Contributor Author

I mean, the user might not know that boot-cljs-devtools needs the file...I agree it can be made clear in the README though...

@Deraen
Copy link
Contributor

Deraen commented May 29, 2016

I do highly recommend creating the .cljs.edn file and setting :require. Without the file boot-cljs naively requires all cljs files is fileset which might cause unused files being included in compilation (slower compilation, can cause surprising side-effects when from required namespaces).

Boot-reload and boot-cljs-repl seem to work though they are before cljs task on the pipeline? Boot-cljs-devtools should be able to work similarly to them.

@arichiardi
Copy link
Contributor Author

Ok so I will ask the maintainer (or do it myself) to make clear that you need an explicit file. Thanks for the clarification, maybe we can make it clear in the README or the wiki for boot-cljs as well?

@Deraen
Copy link
Contributor

Deraen commented May 29, 2016

Ah, now I remember how boot-reload and boot-cljs-repl work.

Boot-reload and boot-cljs-repl similarly search the fileset for existing .cljs.edn and they don't find the file either so they can't add the :require to cljs.edn. The reason why they work but boot-cljs-devtools doesn't, is that they don't use :init-fns.

The generated .cljs.edn file will by default :require all cljs files in fileset and that will include files created by boot-reload and boot-cljs-repl. Because boot-cljs-devtools doesn't create a file in fileset but directly requires devtools namespaces and calls devtools install using init-fns, it doesn't work with the generated cljs.edn.

This is something we could improve in future, but I don't think metadata is a solution.

For now I recommend that boot-cljs-devtools creates a cljs file which requires devtools and calls the install function: https://github.com/adzerk-oss/boot-reload/blob/master/src/adzerk/boot_reload.clj#L34-L44
This file will be picked up by default .cljs.edn.

@arichiardi
Copy link
Contributor Author

Yeah that's exactly the reason and the reason why I was thinking myself of a more generic method...I am PR-ing the boot-cljs-devtools README..

@Deraen
Copy link
Contributor

Deraen commented May 29, 2016

Also, the docs could definitely use improvements regarding cljs.edn file. Looks like it is still documented only under "Multiple builds", which is quite confusing.

@arichiardi
Copy link
Contributor Author

I would make it a first class citizen, promoted to the main README and in Usage as well. It is kind of important 😉

arichiardi added a commit to arichiardi/boot-cljs-devtools that referenced this issue May 29, 2016
@Deraen
Copy link
Contributor

Deraen commented Aug 4, 2017

I guess this is pretty much the same as #169

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

2 participants