Skip to content

Merge all config & programmatic plugins/preset rather than duplicating#6905

Merged
loganfsmyth merged 3 commits intobabel:masterfrom
loganfsmyth:config-options-merge
Nov 28, 2017
Merged

Merge all config & programmatic plugins/preset rather than duplicating#6905
loganfsmyth merged 3 commits intobabel:masterfrom
loganfsmyth:config-options-merge

Conversation

@loganfsmyth
Copy link
Copy Markdown
Member

@loganfsmyth loganfsmyth commented Nov 25, 2017

Q                       A
Fixed Issues? Fixes #6765
Patch: Bug Fix? N
Major: Breaking Change? Y
Minor: New Feature? Y
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

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 .babelrc or a referenced .extends block. 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

{
  presets: [
    ['env', {modules: false}],
  ],
  env: {
    node: {
      presets: [
        ['env', {}],
      ]
    },
  },
}

for example would use {modules: false} by default, but use {} as the options when the env has been set to node for instance. I've also made it so that passing false as 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

presets: [
  '@babel/preset-env',
  require('@babel/preset-env').default,
]

would throw an error for the user about duplicates, and if one was in an env or extends-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

plugins: [
  ['module-resolver', {from: 'view', to: './view' }, "resolve-view"],
  ['module-resolver', {from: 'model', to: './model' }, "resolve-model"],
]

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

presets: [
  'pre',
],
plugins: [
  'plug',
],
env: {
  thing: {
    presets: [
      'pre-env',
    ],
    plugins: [
      'plug-env',
    ],
  }
}

before would have been

  • process pre
  • process plug
  • process pre-env
  • process plug-env

in essentially increasing order, but now they merge first, so it'll be

  • process pre
  • process pre-env
  • process plug
  • process plug-env

which 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

  • values from .extends files, recursively applying the rules here for files
  • the values at the root of the
  • the values from a .env block

and 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.

@loganfsmyth loganfsmyth changed the title Merge all config & programmatic plugins/preset Merge all config & programmatic plugins/preset rather than duplicating Nov 25, 2017
@loganfsmyth loganfsmyth requested review from Andarist and hzoo November 25, 2017 07:28
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Nov 25, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6027/

@existentialism existentialism added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Nov 26, 2017
}
}

return descriptors.reduce((acc, desc) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't feel strongly. I usually just opt for reduce once anything gets complicated. I can change to map + filter too though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt this a private API? maybe we could try to express the test using public API exclusively

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@loganfsmyth loganfsmyth Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe you could just normalize the user's input as soon as possible to use consistent type in the core and avoid $FlowIgnore?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case it is certainly not worth changing right now.

@Andarist
Copy link
Copy Markdown
Member

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

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 :)

Copy link
Copy Markdown
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside a minor change discussed here everything looks good to me 👍

@loganfsmyth loganfsmyth added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Nov 28, 2017
@loganfsmyth loganfsmyth merged commit fba1929 into babel:master Nov 28, 2017
@loganfsmyth loganfsmyth deleted the config-options-merge branch November 28, 2017 21:46
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release PR: New Feature 🚀 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow specifying "mergeMode" for config merging?

4 participants