thheller / shadow-cljs

ClojureScript compilation made easy
https://github.com/thheller/shadow-cljs
Eclipse Public License 1.0
2.27k stars 180 forks source link

feature request: allow matching multiple targets in key of target-defaults map #799

Open alidcast opened 4 years ago

alidcast commented 4 years ago

Currently target-defaults lets you reuse configurations for the same build target, e.g. :browser, but it does not let you reuse configurations for separate yet related build targets, e.g. :browser and :browser-test.

As a solution for this, would you consider a PR that allowed :target-defaults to match keywords in a set (or any collection)?

My use-case is sharing :js-options across related browser build targets:

 :target-defaults
 {:browser
  {:js-options
   {:extensions [".web.js" ".js" ".json"]
    :resolve {"react-native" {:target :npm
                     :require "react-native-web"}}}}
  :browser-test
  {:js-options
   {:extensions [".web.js" ".js" ".json"]
    :resolve {"react-native" {:target :npm
                              :require "react-native-web"}}}}}}

If :target-defaults matched build target configurations for sets, then the above could be:

 :target-defaults
 {#{:browser :browser-test}
  {:js-options
   {:extensions [".web.js" ".js" ".json"]
    :resolve {"react-native" {:target :npm
                     :require "react-native-web"}}}}

One consideration is ordering of merges if multiple :target-default are matched. Below ~8 items maps are an array-map, but still since Clojure maps don't guarantee order, this could give false assumptions if somehow someone's config unexpectedly got too big.

What do you think? Have you considered allowing multiple targets in a :target-default entry? Do you have other ideas for how to address shared configs for related build targets?

thheller commented 4 years ago

How many of these do you have? A little duplication is not a problem and does not warrant complicating the implementation too much.

I'm not a fan of using a set as the map key. Instead maybe create a different mechanism where each config can refer to a set of defaults by name or so.

:custom-defaults
{:foo {:js-options ...}}

:builds
{:a {:target :browser :use-defaults [:foo] ...}
 :b {:target :browser-test :use-defaults [:foo] ...}

Not sure this is much better though. Just a thought.

alidcast commented 4 years ago

Right now I just have that one duplication of :browser and :browser-test configs. But as I build for different platforms (desktop, mobile, tests in CI), there'll likely be more config duplication.

I like the ease of use of matching a set of keys by their build target. But with the :custom-defaults option you suggested config extensions are more explicit and merging would be more straightforward since it'd be a vector of keys; so it does seem like a better approach.

Should I just stick with the duplication for now, or would you like a PR that adds the :custom-defaults option?