Fixed useBuiltIns and modules validation when using 'false' as option

[JMarkoski]

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

Fixes the case when 'false' as string is provided for useBuiltIns or modules option by not converting the supplied option to string when it's 'false', and comparing it directly.

[existentialism]

@JMarkoski thanks for the PR, and congrats on your first contribution 🙏! can we add a test for this here?

[JMarkoski]

@existentialism Thanks, I'm very happy to contribute! I've added the missing tests for validateUseBuiltInsOption and for validateModulesOption when the input is 'false'. I think it's okay now.

[nicolo-ribaudo]

Thanks!

[existentialism]

@JMarkoski great job!

[bencripps]

Is this considered a breaking change?

I notice that for our application, we now have a bunch of broken builds because we aren't setting this property explicitly.

Even more troublesome is that for dependencies that we need to transpile, we also run into this issue because their local babel-config files take precedence over the application specific configurations.

In our builds we now see the following errors:

   Invalid Option: The 'modules' option must be one of
     - 'false' to indicate no module processing
     - a specific module type: 'commonjs', 'amd', 'umd', 'systemjs' - 'auto' (default) which will automatically select 'false' if the current
       process is known to support ES module syntax, or "commonjs" otherwise

Our babel config looks as follows:

module.exports = {
    presets: [['@babel/preset-env', { modules: true }], '@babel/preset-react']
};

[JLHwung]

Per https://babeljs.io/docs/en/babel-preset-env#modules, preset-env never accepts true as a valid option. As the error message is shown, please specify the module type that your app targets to.

Besides, the only behaviour change introduced in this PR is throwing on modules: "false", which is also never accepted but in previous versions it is processed as modules: false, this is considered a bug fix.

Should you have more questions, please submit a new issue. Thanks.

[JMarkoski]

Also, just to add, you say that you don't set the option explicitly, but in the configuration you set it to {modules: true} which is an invalid option. If you don't set the modules option explicitly it will automatically select false if the current process is known to support ES module syntax, or 'commonjs' otherwise, as the error says.

[bencripps]

Eee sorry, that was a mistype, from me trying to debug the issue. Our actual config is:

module.exports = {
    presets: ['@babel/preset-env', '@babel/preset-react']
};

Previously we were on @babel/preset-env@7.8.7 -- when we upgraded to @babel/preset-env@7.9.5 we started seeing the error above.

[JLHwung]

module.exports = {
    presets: ['@babel/preset-env', '@babel/preset-react']
};

In the config above, preset-env will assume modules: "auto" and I can not reproduce the error you mentioned.

we also run into this issue because their local babel-config files take precedence over the application specific configurations.

Can you share their local babel config files? Or better can you construct a reproduction repo? This can greatly help investigate the issues.

[bencripps]

Their local file looks as follows:

{
    "presets": ["es2015", "react", "stage-2"],
    "compact": false,
    "env": {
      "test": {
        "plugins": ["transform-runtime"],
        "sourceMaps": "both"
      }
    }
}

I can try to construct a reproducible repo. Curious is the assumption of "auto" a new behavior, or has it always been this way?

[bencripps]

It's also interesting to note, that if I comment out the line check in validateModulesOption, found in the newest @babel/preset-env the build succeeds, without any compilation errors, which makes me think we don't have a code issue, and it's just this check which is throwing.

const validateModulesOption = (modulesOpt = _options.ModulesOption.auto) => {
  // comment out the validation 
  // (0, _invariant.default)(_options.ModulesOption[modulesOpt.toString()] || modulesOpt === _options.ModulesOption.false, `Invalid Option: The 'modules' option must be one of \n` + ` - 'false' to indicate no module processing\n` + ` - a specific module type: 'commonjs', 'amd', 'umd', 'systemjs'` + ` - 'auto' (default) which will automatically select 'false' if the current\n` + `   process is known to support ES module syntax, or "commonjs" otherwise\n`);
  return modulesOpt;
};

[bencripps]

I can confirm that the issue doesn't happen in our source code, and that the error only happens because one of our dependencies is relying on a previous version of the babel libraries (as you can see from the above config).

Is it possible that this change causes an unexpected issue for our (admittedly odd) use-case?

Unfortunately, we don't have access to change their code to use a newer version of babel.

[JMarkoski]

Curious is the assumption of "auto" a new behavior, or has it always been this way?

I can confirm that @babel/preset-env@7.8.7 and @babel/preset-env@7.9.5 both have the 'auto' option. Also, babel-preset-es2015 only supports "commonjs", "amd", "umd", "systemjs", false values for the modules option, but I don't know if that's related, I am relatively new to the project and only familiar with a part of the source code.

Apart from that, I don't know what might be the issue, can you construct a reproduction repo to investigate further?

[bencripps]

I've tried to build a minimally reproducible version but it's tough since many of the packages (including dev dependencies) are private.

However, in my debugging to see what "value" is causing the problem, I updated the error statement to include some extra information. It looks like it's throwing, but that the value it's getting is correct? It seems to be "false", of type string, which should be allowed?

Invariant Violation: [BABEL] /Users/bcripps/indeed/mosaic-provider-base-nodejs/node_modules/@indeed/frontend-components-react/components/Modal/index.js: Invalid Option:
    ArgValue: false
    TypeOf Arg: string 
    The 'modules' option must be one of
     - 'false' to indicate no module processing
     - a specific module type: 'commonjs', 'amd', 'umd', 'systemjs' - 'auto' (default) which will automatically select 'false' if the current
       process is known to support ES module syntax, or "commonjs" otherwise

[JMarkoski]

Then this is the expected behavior. This PR fixes exactly this issue, where you specify 'false' as the option which is string instead of a boolean false. Previously if you specified 'false' as a string it was fine, with this fix it should throw an error as in your example.

[bencripps]

Ah interesting! When I add this

  console.log('typeof moduleOpt ' +  typeof modulesOpt)

I see

typeof moduleOpt string
typeof moduleOpt string
typeof moduleOpt string
typeof moduleOpt string
typeof moduleOpt string
typeof moduleOpt string
typeof moduleOpt string
typeof moduleOpt string

but none of our configs specify this parameter at all? Do you know what might be converting it to "false"?

[bencripps]

I found it! We have some (🤦) internal build code that's injecting a string value :/

Thank you so much for your help in debugging this!

[JMarkoski]

Great! You are welcome, I am very glad if I helped.