boot-clj / boot-cljs

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

add cljs-edn generator #142

Closed burn2delete closed 7 years ago

burn2delete commented 8 years ago

@Deraen @martinklepsch any comments on this version?

martinklepsch commented 8 years ago

@flyboarder are you using this task instead of committing a .cljs.edn file to your project? I initially understood this as a task to generate it so it gets saved to your resources directory and essentially helps users adopt the usage of .cljs.edn files. That said I can see that you could also just skip having a .cljs.edn in your source tree using this task which some might find handy. Probably something one has to use to get a feel for.

burn2delete commented 8 years ago

@martinklepsch My usage is to avoid the need to committing additional files to the project directory. (However they are included in output so users can learn from them) Instead these files are generated as needed, my current usage is multiple different build modes. Or for specific platform like in boot-electron and boot-nodejs https://github.com/degree9/boot-nodejs/blob/master/src/degree9/boot_nodejs.clj#L70-L80

martinklepsch commented 8 years ago

Ok that makes sense 👍 given that most people don't use target they won't see the file. What do you think about an option like persist that takes the first of :resource-paths and stores the file there? This extends the scope of what you wanted to do initially quite a bit but will also make it much more useful to regular users that don't have the same scenario you're working on.

burn2delete commented 8 years ago

@martinklepsch Are you referring to adding it to the fileset during a build pipeline, or persisting it to the project directory? Im weary of mutating the project, but perhaps a second task that uses boot-new generators could also be whipped up for mutating the project folders??

martinklepsch commented 8 years ago

Are you referring to adding it to the fileset during a build pipeline

You're doing that already if I understand correctly, that's good.

or persisting it to the project directory

That's what I'm referring to, yes. I understand it might be a bit weird to modify the project but it would be an opt-in so I'm thinking it would be fine. Nice thing here would be that we could give users a snippet and they'd get a cljs.edn generated without knowing too much about what it needs to look like.

martinklepsch commented 8 years ago

@flyboarder if you think the option to add the cljs.edn to the project directory is a terrible idea I won't be in the way of merging it, just came into my mind when I saw it.

burn2delete commented 8 years ago

@martinklepsh I like the idea, I think I'll work it in using the boot-new generators tho, I'd like to do that in a separate PR tho as to avoid blocking this.

burn2delete commented 7 years ago

@martinklepsch A 🎁 for you! Generators included!

Updated: Needs latest version of boot-new (master), not ready to merge.

Updated Again: Now ready!

martinklepsch commented 7 years ago

Hah, thank you very much for the early present @flyboarder 😊

Would you mind logging these additions into the Changelog and perhaps adding some usage examples to the wiki?

@Deraen what do you think about integrating the wiki into the repo as doc/ or similar? Would make it easier to keep code and docs in sync and make sure when new features are added they are also documented properly.

Deraen commented 7 years ago

Okay,

In general I'm still not fan of this task. But the PR is clean and this doesn't affect any other code so I think this is acceptable.

There are a few small things that could be changed, as I commented.

Docs are definitely necessary. I tried testing the generator but didn't find any way to call that, since boot-new presumes that the template name refers to dependency name.

burn2delete commented 7 years ago

@Deraen I removed the duplicate options from the task, I also moved the generator to boot-new to keep the tasks separate

Deraen commented 7 years ago

After adding support for :output-to, :output-dir and :main, I think this is largely unnecessary now.

All the options can now be set from task options without .cljs.edn file.

I will still need to change default-main to not include all the cljs files in :require automatically.

burn2delete commented 7 years ago

Agreed! Thanks so much for your work 👍