New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying "mergeMode" for config merging? #6765

Closed
loganfsmyth opened this Issue Nov 7, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@loganfsmyth
Member

loganfsmyth commented Nov 7, 2017

We've got #5276 filed, but the main reason for removing env is that users simply don't understand how the merge logic works. (And I don't blame them, I barely understand it, and might not be able to describe it to you.

That said, there are other places where we merge, so we should still figure out a better story around how configurations get merged together. Primary cases where we merge now:

  • Config files can extends: "./other-config" and things get merged
  • Programmatic options to babel.transform are merged with .babelrc options
  • "env" blocks are merged with their parent configs
  • If we do #5451, we need a clear story around merging for specific files.
@clemmy

This comment has been minimized.

Show comment
Hide comment
@clemmy

clemmy Nov 22, 2017

Contributor

From reading the related issues, I'm unsure of what mergeMode would look like, and how you propose it would solve the story about how configurations are merged together. Can you elaborate?

Contributor

clemmy commented Nov 22, 2017

From reading the related issues, I'm unsure of what mergeMode would look like, and how you propose it would solve the story about how configurations are merged together. Can you elaborate?

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 22, 2017

Member

I'm in the process of implementing a prototype of this right now. I kind of decided making an option to toggle was just not worth it, so Babel 7 may very well just stabilize with an entirely different config merging process :D

What I'm exploring would essentially allow for users to do

{
  plugins: [
    ['some-plugin', {one: true, two: true}],
  ],
  env: {
    test: {
      plugins: [
        ['some-plugin', {two: false, three: false,}],
      ],
    }
  }
}

would only be a single instance of the plugin, with the options from test merged into the once from the root, essentially being

{
  plugins: [
    ['some-plugin', {one: true, two: false, three: false}],
  ],
}

when the environment was set to test.

Member

loganfsmyth commented Nov 22, 2017

I'm in the process of implementing a prototype of this right now. I kind of decided making an option to toggle was just not worth it, so Babel 7 may very well just stabilize with an entirely different config merging process :D

What I'm exploring would essentially allow for users to do

{
  plugins: [
    ['some-plugin', {one: true, two: true}],
  ],
  env: {
    test: {
      plugins: [
        ['some-plugin', {two: false, three: false,}],
      ],
    }
  }
}

would only be a single instance of the plugin, with the options from test merged into the once from the root, essentially being

{
  plugins: [
    ['some-plugin', {one: true, two: false, three: false}],
  ],
}

when the environment was set to test.

@clemmy

This comment has been minimized.

Show comment
Hide comment
@clemmy

clemmy Nov 22, 2017

Contributor

Interesting, I am digging this change. One thing, however, is I don't see how it solves #5451.

Contributor

clemmy commented Nov 22, 2017

Interesting, I am digging this change. One thing, however, is I don't see how it solves #5451.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Nov 22, 2017

Member

@clemmy One step at time 😄 Getting the config merging logic right is necessary for that proposal.

Member

nicolo-ribaudo commented Nov 22, 2017

@clemmy One step at time 😄 Getting the config merging logic right is necessary for that proposal.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 22, 2017

Member

Yeah, doesn't solve that, it's just a blocker because without better merging, having override behavior would be an absolute minefield for users.

Member

loganfsmyth commented Nov 22, 2017

Yeah, doesn't solve that, it's just a blocker because without better merging, having override behavior would be an absolute minefield for users.

@clemmy

This comment has been minimized.

Show comment
Hide comment
@clemmy

clemmy Nov 22, 2017

Contributor

Okay! Thanks for the context.

Contributor

clemmy commented Nov 22, 2017

Okay! Thanks for the context.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Nov 22, 2017

Member

More and more I think about it - it seems like unnecessary complication. While some might dig the proposed merge strategy, some will certainly find it confusing. IMHO it's better to have people to specify exactly what they want in @loganfsmyth's example for test env and that is {one: true, two: false, three: false}.

Babel7 will come with first class support for .babelrc.js and that means that anyone can implement his/her own merging strategy with custom javascript logic. Leaving auto-merges out of Babel's codebase would simplify it and would be easier to maintain.

Member

Andarist commented Nov 22, 2017

More and more I think about it - it seems like unnecessary complication. While some might dig the proposed merge strategy, some will certainly find it confusing. IMHO it's better to have people to specify exactly what they want in @loganfsmyth's example for test env and that is {one: true, two: false, three: false}.

Babel7 will come with first class support for .babelrc.js and that means that anyone can implement his/her own merging strategy with custom javascript logic. Leaving auto-merges out of Babel's codebase would simplify it and would be easier to maintain.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Nov 22, 2017

Member

True! That's certainly an option too. I guess I was thinking that merging would be a more flexible strategy, like if someone wanted to do a full replace they could do

plugins: [
  ['some-thing', { some: {nested: 'thing'}}],
],
env: {
  test: {
    plugins: [
      ['some-thing', false],
      ['some-thing', { some: {nested: 'thing'}}, 'newname'],
    ],
  }
}

so use false to disable the one at the root, and then make a new one with the name newname.

Doing a full replace does have some nice benefits though, since it means we'd have a clear 'dirname' for the options given to the plugin, so I'd definitely be open to it if people think it would be a better bet for people. I definitely agree that it would be simpler to describe.

Member

loganfsmyth commented Nov 22, 2017

True! That's certainly an option too. I guess I was thinking that merging would be a more flexible strategy, like if someone wanted to do a full replace they could do

plugins: [
  ['some-thing', { some: {nested: 'thing'}}],
],
env: {
  test: {
    plugins: [
      ['some-thing', false],
      ['some-thing', { some: {nested: 'thing'}}, 'newname'],
    ],
  }
}

so use false to disable the one at the root, and then make a new one with the name newname.

Doing a full replace does have some nice benefits though, since it means we'd have a clear 'dirname' for the options given to the plugin, so I'd definitely be open to it if people think it would be a better bet for people. I definitely agree that it would be simpler to describe.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Nov 22, 2017

Member

Also in most cases when things are simpler to describe they are also easier to grok by people which in turn should spare us more questions on the issues from confused people.

Member

Andarist commented Nov 22, 2017

Also in most cases when things are simpler to describe they are also easier to grok by people which in turn should spare us more questions on the issues from confused people.

@lock lock bot added the outdated label May 3, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.