shadow-cljs icon indicating copy to clipboard operation
shadow-cljs copied to clipboard

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

Open alidcast opened this issue 3 years ago • 2 comments

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?

alidcast avatar Oct 19 '20 04:10 alidcast

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.

thheller avatar Oct 19 '20 11:10 thheller

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?

alidcast avatar Oct 19 '20 18:10 alidcast