Merge all config & programmatic plugins/preset rather than duplicating#6905
Merge all config & programmatic plugins/preset rather than duplicating#6905loganfsmyth merged 3 commits intobabel:masterfrom
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6027/ |
e29dd91 to
662ce7e
Compare
662ce7e to
189c566
Compare
| } | ||
| } | ||
|
|
||
| return descriptors.reduce((acc, desc) => { |
There was a problem hiding this comment.
wouldnt this be clearer as filter? would have to be filter + map though in this case, cant we push to descriptors only valid entries? or are descriptors' items being mutated when already in the array?
There was a problem hiding this comment.
Yeah I don't feel strongly. I usually just opt for reduce once anything gets complicated. I can change to map + filter too though.
There was a problem hiding this comment.
Dont feeling strongly about this either, just leave it in whatever shape you want :)
| const { calls: calls1, plugin: plugin1 } = makePlugin(); | ||
| const { calls: calls2, plugin: plugin2 } = makePlugin(); | ||
|
|
||
| manageOptions({ |
There was a problem hiding this comment.
isnt this a private API? maybe we could try to express the test using public API exclusively
There was a problem hiding this comment.
Yeah I can rename this. This is the same public API you can get from babel.loadOptions so I think I can change these tests to use that.
| if (Array.isArray(value)) { | ||
| [value, options] = value; | ||
| if (value.length === 3) { | ||
| // $FlowIgnore - Flow doesn't like the multiple tuple types. |
There was a problem hiding this comment.
is the length check necessary here? you could just unpack the tuple here as is and the name would simply stay undefined, although we shouldnt access out of bounds elements, so maybe it would be reasonable to have only 1 tuple type for this, where the last element would be a maybe type AND it would require explicit third element of null/undefined
There was a problem hiding this comment.
where the last element would be a maybe type AND it would require explicit third element of null/undefined
I've tried to keep the data types in parallel with what people would actually have in their, so I kept them as two separate types.
There was a problem hiding this comment.
Hm, maybe you could just normalize the user's input as soon as possible to use consistent type in the core and avoid $FlowIgnore?
There was a problem hiding this comment.
I can't do that currently because copying the input object would invalidate some of the plugin caching, that said I do think it is confusing right now. Perhaps something to revisit in the future?
There was a problem hiding this comment.
In that case it is certainly not worth changing right now.
I don't know many use cases when one would want multiple instances of the same plugin, I think most use cases should be covered by plugin's options, but possibly there are some cases when that's desirable and the proposed solution seems clean to me. Personally I like the proposed behaviour, seems not complicated - both for users and the codebase. So 2 👍 from me :) |
In an effort to make config a bit closer to what people generally think Babel already does, this makes Babel's config-level options merge. That means everything passed in programmatically, and loaded via a
.babelrcor a referenced.extendsblock. Plugins from a config will not merge with plugins inside of a preset itself, since names and values of plugins inside presets are essentially private implementation defined data.This means
for example would use
{modules: false}by default, but use{}as the options when theenvhas been set tonodefor instance. I've also made it so that passingfalseas the options value for a preset or plugin will disable that item entirely.I should also mention that things merge based on the identity of the plugin function itself. That means that
would throw an error for the user about duplicates, and if one was in an
envorextends-file, they would merge happily. It seemed like the safest way to ensure that someone didn't refactor and break their config or something.Importantly the feature that is no longer possible with this is using the same plugin twice. To allow for that, this PR includes logic to allow users to name specific plugins to uniquely identify them by expanding the 2-tuple to an optional 3-tuple, as
which would then not override eachother since they have different names.
Not super pretty, but also probably not super common so potentially not a real issue. Definitely curious for thoughts on that one.
Breaking Changes
Plugin/preset evaluation ordering
The obvious breaking change is obviously that things won't be duplicated anymore, but the potentially less obvious point is that things potentially change order. That said, I think the order is better this way. The merging happens up front for both plugins and presets, so where before each nested config was essentially merged into the plugin list in order, now they merge first then execute. So for example
before would have been
preplugpre-envplug-envin essentially increasing order, but now they merge first, so it'll be
prepre-envplugplug-envwhich to me is what people expect more anyway, but it is what caused most the the unittest changes that you'll see with this PR.
The priority of a given config object goes in increasing order of
.extendsfiles, recursively applying the rules here for files.envblockand values passed in via the programmatic options will merge with a higher priority, overriding values from config files.
Preset options
Presets can provide options the same way config files can, but it isn't something that is done super often. That said, it complicates merging because for instance it isn't obvious what priority the preset config values would have compared to the user's config files. This PR officially changes the semantics of preset options to be default values, so user config will automatically override any default value that a preset has specified.