Use deep merge for :compiler-options

[zlorf]

Using deep merge for :compiler-options would allow to merge :closure-defines from task definition and .cljs.edn files instead of replacing one by another.

It'd be especially usefull for :parallel-build.

[Deraen]

I'm somewhat conflicted about using deep-merge here, it can potentially make it harder to undefine some options for production, which are set as default for development. We can probably prevent this with some log messages.

I wonder if we should manually define the merge strategy per option?

• :foreign-libs - into
• :externs - into
• :preloads - into or replace because sometimes it can be necessary to reset the preloads option
• :modules - merge? or replace
• :warnings merge
• :libs into
• :preamble into or replace
• :closure-warnings merge
• :closure-defines merge
• :closure-extra-annotations ?

Leiningen allows using ^:replace metadata per value to control how data is merged. Those are not available to us as we are using edn files as one source of data. Though those could maybe be used with the task options.

[zlorf]

Leiningen-like hints like ^:replace would be great and sufficient, and it will allow to either merge or replace (or undefine) each option.
And since task options take precedence (override) edn options (since last PR), it's only meaningful to use ^:replace in tasks (since edn don't overwrite anything). And that's exactly what we want.

[Deraen]

A small problem is that the metadata can't be provided when using command line options. But I guess that doesn't matter, if user needs to use metadata, one can add the options to build.boot.

[zlorf]

Agreed.
To maintain current behaviour, the 'replace' strategy should be a default one, and 'merging' should be triggered by a metadata keyword, something like ^:join or ^:merge (it'd be the opposite way of Leiningen, where merging is default).

[Deraen]

The change to option precedence already broke backwards compatibility so I don't think it would be too bad to use merge as default for maps. I think replace would be better default for sequence like types, but I guess it would be good to check leiningen.

[kennyjwilli]

@micha and I had a conversation about this last month. See https://clojurians-log.clojureverse.org/boot/2016-08-04.html. The conclusion we drew was:

The conclusion I have drawn is that tasks will generally not have a complex map for specifying options. If the task does need a map to specify options it should be set in a task-specific file that can be overridden by task-options. If the task-specific file is overridden by task options, it should be completely overridden so you don't introduce any additional complexity of merging.

[zlorf]

@kennyjwilli So I will provide a counter-example for your conclusion, based on my current need. It is something like you've described in discussion, but connected to :closure-defines.
I have one codebase that gets compiled into browser script and node script. There is goog-define flag called nodejs? that is being set (in :compiler-options :closure-defines) to false in browser.cljs.edn and to true in node.cljs.edn. This way advanced optimizations cut the dead code for every (if nodejs? (do-something-on-node-only) (do-something-in-browser-only)). So far so good.
build.boot contains only one to call (cljs :optimizations :advanced :compiler-options {:parallel-build true}) that produce two scripts based on their edn files.
However, I wanted to introduce a new goog-define flag (let's call it test-environment?) that would depend on some config file (e.g. dev/production) and trigger another "conditional compilation" via dead code elimination. But if I use it like (cljs :optimizations :advanced :compiler-options {:parallel-build true :closure-defines {"foo.bar.test_environment_Q_MARK" <true/false - value read from config file>}}), then then nodejs? flag is not set, because :closure-defines in edn files were overwritten.
I cannot read config files in edn, and splitting cljs task in build.boot into two separate tasks just to set nodejs? flag seems wrong.
So what I'd like is a possibility to combine task-specific (in terms of edn file) :closure-defines with another :closure-defines that are related to selected config file.

I'd like to do something like (deftask doit [] (comp (cljs) (cljs-compiler-options :update #(update-in % [:closure-defines] into ['foo.bar "asdf"])))) if merging doesn't happen by default, but it seems that it is not possible now.

[kennyjwilli]

From what I understand (correct me if I'm wrong), merging maps in boot-cljs is not as simple as it sounds because it wraps each property passed to the compiler. I believe it could be done but it would take a significant amount of work.

Anyway, your use case is almost identical to the one I have and I still believe the update-file option would work for you. I should have also mentioned how we wanted to make this easier for boot users going forward by adding this ability to the built in task sift. This would allow you to regex match a file and get the content passed, as a stream, to a handler function, also taking an output stream. You would write the new contents of the matched file to the output stream and the cljs task would pick up that file later in the pipeline. Of course you probably don't want to be messing with streams when your goal is simple -- you just want to update the map data structure in an edn file -- so we would provide handy macros for this sort of thing (e.g. with-edn).

The task I wrote as a starting place is located here.

[zlorf]

Merging maps shouldn't be tricky: I think it's just a matter of replacing merge by merge-with https://github.com/adzerk-oss/boot-cljs/blob/master/src/adzerk/boot_cljs/middleware.clj#L43

Thank you for the update-file code; I'll give it a try, but for now it seems to be able to resolve my use case.

[zlorf]

@kennyjwilli It seems to be working, however I needed to add make-parents call (proper comment in the gist).

[kennyjwilli]

Thanks. This will be added to to the sift task in the end.