Skip to content
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 configs to have an 'overrides' array #7091

Merged
merged 4 commits into from
Jan 7, 2018

Conversation

loganfsmyth
Copy link
Member

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

This does two related things

  1. Allow every config object to specify a test/include/exclude field, just like you might do for Webpack. Each item allows an item, or array of items that can be a string, RegExp, or function.

    Note, this means you could actually limit a .babelrc to only apply to some files inside of a directory, or make an env override only apply to certain files in that env, or something, which seemed like it was reasonable to allow and would be an unnecessary restriction to disallow.

  2. Allow an overrides array of sub-configs that apply on top of the base configs that will apply based on their test/include/exclude values.

As an example, this allows user to do

{
  presets: [
    ['env', { 
      targets: { node: 'current' },
    }],
  ],
  overrides: [{
    test: "./client",
    presets: [
      ['env', { 
        targets: { browsers: ["last 2 versions"] },
      }],
    ],
  }],
}

or for instance we'll use this in Babel's own codebase to do

{
  presets: [
    ['env', {modules: false}],
  ],
  plugins: [
    ['@babel/transform-modules-commonjs', { lazy: true }],
  ],
  overrides: [{
    test: './packages/babel-register',
    plugins: [
      // Override the plugin with an empty module options config to disable lazy modules.
      '@babel/transform-modules-commonjs',
    ],
  }],
}

to enable lazy modules for everything except babel-register, where it breaks things.

@loganfsmyth loganfsmyth added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Dec 22, 2017
@loganfsmyth
Copy link
Member Author

This doesn't have any tests, but I'll try to add some tomorrow.

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 22, 2017

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

@@ -419,7 +546,7 @@ function matchesPatterns(

const absolutePatterns = strings.map(pattern => {
// Preserve the "!" prefix so that micromatch can use it for negation.
const negate = pattern[0] === "!";
const negate = pattern[0] === "!" && !disableNegation;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should throw an error/report a warning if disableNegate && pattern[0] === "!"

Copy link
Member

Choose a reason for hiding this comment

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

!disableNegation is not super clear to me but I don't have a better idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, happy to throw instead.

@@ -216,6 +243,9 @@ export function validate(type: OptionsType, opts: {}): ValidatedOptions {
if (type === "env" && key === "env") {
throw new Error(`.${key} is not allowed inside another env block`);
}
if (type === "env" && key === "override") {
throw new Error(`.${key} is not allowed inside an env block`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we also disable override in another override block?

Copy link
Member

Choose a reason for hiding this comment

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

I would say yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, definitely an oversight.

@hzoo
Copy link
Member

hzoo commented Dec 22, 2017

Awesome really looking forward to this one! Yeah tests and some docs like https://eslint.org/docs/user-guide/configuring#configuration-based-on-glob-patterns will be useful

Allow every config object to specify a test/include/exclude

Does this include the top level config without the overrides or only in the overrides array config objects? And how do we expect this to work with only/ignore? Although I know they are different since one is about processing the files at all and the other is to change behavior

@loganfsmyth
Copy link
Member Author

Does this include the top level config without the overrides or only in the overrides array config objects?

It includes any config object, including top-level, and env and those in overrides.

And how do we expect this to work with only/ignore?

Currently, test/ignore/exclude are processed first, so if the test doesn't match, the ignore in that config object would never be applied.

Essentially I'm separating them as ignore/only means that a file essentially doesn't get transformed at all, whereas test/... says "don't use this specific config object".

@loganfsmyth
Copy link
Member Author

I think this should be good now.

@hzoo
Copy link
Member

hzoo commented Jan 5, 2018

Would be nice to be able to test this locally on our repo before merging (like all the other config changes)

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

good with this based on tests (tbh having a hard time looking through the code with all the caching, etc)

Allow every config object to specify a test/include/exclude field, just like you might do for Webpack. Each item allows an item, or array of items that can be a string, RegExp, or function.

Not sure we want to introduce all these different ways of doing it though? array/non-array, string, regex,fn, include/exclude/test

@loganfsmyth
Copy link
Member Author

Not sure we want to introduce all these different ways of doing it though? array/non-array, string, regex,fn, include/exclude/test

I went with it since it seemed like an established pattern in Webpack that people would be able to not have to think about a ton. The rest was taken from what we already did for ignore/only. Was there a trimmed down set of functionality you'd prefer?

@hzoo
Copy link
Member

hzoo commented Jan 6, 2018

I guess it's hard to know all usecases, so not sure - like monorepo/not, app/library, webpack/node, etc. Nothing in particular, if we think that kind of parallel is good? Just wondering if it's confusing to have that kind of nested level of config but I guess doesn't mean people are going to write it that way. We probably want to provide some level of docs on basic/advanced configs with webpack soon

@loganfsmyth
Copy link
Member Author

Just wondering if it's confusing to have that kind of nested level of config but I guess doesn't mean people are going to write it that way.

Webpack is one of the few cases where people can have fine-grained control of transforms. I imagine this config will be more likely in use for people using the CLI or gulp or any number of other integrations. I also think it could be nice for Webpack since you can still centralize all your babel configs at least.

@hzoo
Copy link
Member

hzoo commented Jan 6, 2018

Ok cool, sounds good.

not really specific to the pr but: something that I was thinking of as a result of this is being able to get the files/transforms run in a debug mode for ourselves/others to better understand what babel is actually doing

(options.test === undefined ||
configFieldIsApplicable(context, options.test, dirname)) &&
(options.include === undefined ||
configFieldIsApplicable(context, options.include, dirname)) &&
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between test and include?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's the same actually just matching webpack

Choose a reason for hiding this comment

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

Old webpack docs are gone, but I believe it used to be that test had to be a regex (e.g. /\.js$/), while include/exclude had to be a string (e.g. srcDir)? In any case, they have gained the connotations of "file type" and "directories".

throw new Error(`.${key} is not allowed inside an env block`);
}
if (type === "override" && key === "overrides") {
throw new Error(`.${key} is not allowed inside an overrides block`);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You can write throw new Error(".overrides is not allowed inside an overrides block");, since key is guaranteed to be overrides because of the line above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I just felt like it was more consistent this way.

@loganfsmyth loganfsmyth merged commit a19349a into babel:master Jan 7, 2018
@loganfsmyth loganfsmyth deleted the config-overrides branch January 7, 2018 21:51
@novemberborn
Copy link
Contributor

Is it expected that the overriding options can contain an extends clause, and the extended config can itself contain overrides, which are then evaluated?

Validation of overrides prevents such nesting, but it's currently allowed through extends. I've put an example up at https://gist.github.com/novemberborn/fdf830ce929a8641651f7838ea4efab1, compare npx babel bar.js with npx babel foo.js.

I'm not saying this behavior is necessarily wrong but it'd be good to know whether it's intentional.

@xtuc
Copy link
Member

xtuc commented Jan 16, 2018

Sorry for being late at this, but super job @loganfsmyth!

@simonbuchan
Copy link

Reposting for visibility from my referencing comment above:

With this babel should now be able to behave like tsc, as babylon is perfectly happy to parse <type> expr with syntax-typescript so long as syntax-jsx is not also enabled.:

const common = ["@babel/env"];
module.exports = {
  overrides: [
    { test: /\.js$/, presets: common },
    { test: /\.jsx$/, presets: [...common, "@babel/react"] },
    { test: /\.ts$/, presets: [...common, "@babel/typescript"] },
    { test: /\.tsx$/, presets: [...common, "@babel/typescript", "@babel/react"] },
  ]
}

Probably something to suggest in whatever typescript docs get added? What's the roadmap on docs anyway?

@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: 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.

[Feature Request] feature similar to ESLint "overrides" (config based on glob)
7 participants