[preset-env] Include / exclude module plugins properly

[AdamRamberg]

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

This PR fixes how includes and excludes of module plugins (proposal-dynamic-import, transform-modules-commonjs, transform-modules-amd, transform-modules-commonjs, transform-modules-systemjs, transform-modules-umd) works. After this PR it is possible to exclude module plugins, but not include them.

Before this PR it wasn't possible to exclude any of the plugins listed above. This caused issues when you wanted to transform ES modules, but not dynamic imports (see this issue).

Another case that this PR is fixing is that it was possible to include a module plugin. However, that could potentially cause issues where a plugin was added twice, eg. with the following config:

{
    presets: [
      [
        '@babel/preset-env',
        {
          modules: 'cjs',
          include: ['@babel/plugin-transform-modules-commonjs'],
        },
      ],
    ],
  }

Adding this PR will make it possible to set modules: 'cjs' and exclude: ['transform-modules-commonjs'], which will resolve in that modules are not transformed to commonjs. A warning or an error might be preferable here, but I think that this behaviour is better then it was before (before it just transformed modules to commonjs). Furthermore, unless someones babel config is ambiguous (see cases above), this change should not break anyones' builds.

[babel-bot]

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

[babel-bot]

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

[JLHwung]

The filterItems takes care of the include/exclude functionalities. #10194 breaks since filterItems is not the last plugins processing step. Any processing steps following the filterItems which manipulates the pluginsList, would introduce include/exclude issues like #10194.

I am wondering if we can reuse the defaultIncludes in filterItems and have shouldTransformESM and shouldTransformDynamicImport works on a new defaultIncludes array, later we pass it to filterItems and include/exclude should work. In this approach we do not introduce extra logic in the codebase other than filterItems to handle the include/exclude pairs.

let defaultIncludes = [];
if (modules !== false && moduleTransformations[modules]) {
  // figure out an array of relatedPlugins
  defaultIncludes.push(...)
})

const transformations = filterItems(
    shippedProposals ? pluginList : pluginListWithoutProposals,
    include.plugins,
    exclude.plugins,
    transformTargets,
    defaultIncludes,
    getOptionSpecificExcludesFor({ loose }),
    pluginSyntaxMap,
);

Note that filterItems support Array<string> type of defaultIncludes, you may consider extend its functions so that it works for Array<string | [string, Object]>.

We may also add a comment that filterItems should be the last step to determine an array of plugins. Otherwise include/exclude would fail on the extra plugins inserted by any processing step.

[AdamRamberg]

@JLHwung Great points! Did struggle with this when doing the PR and was thinking about how I could do all of the filtering in filterItems. Left it like this since there were already filtering going outside of filterItems before this PR with shouldTransformESM and shouldTransformDynamicImport. Much cleaner to keep all the filtering / selecting in filterItems though. Will create a new commit with the adjustments you suggests above.

[AdamRamberg]

Got a question, after looking into @JLHwung suggestions above. Preset-env is currently handling module plugins'/transformers' options differently compared to other plugins, just passing along the loose option:

    if (shouldTransformESM) {
      // NOTE: not giving spec here yet to avoid compatibility issues when
      // transform-modules-commonjs gets its spec mode
      plugins.push([getPlugin(moduleTransformations[modules]), { loose }]);
    }

and

if (
      shouldTransformDynamicImport &&
      shouldTransformESM &&
      modules !== "umd"
    ) {
      plugins.push([getPlugin("proposal-dynamic-import"), { loose }]);
    }

From what I can tell from the docs there are no module plugins that are currently using any of the options passed to the other plugins (spec and useBuiltIns), but omitted to the module plugins. So from my understanding passing a long spec and useBuiltIns to the module plugins should not affect anything. Passing along the same options to these plugins would also reduce code complexity so I would really like to do this. I could however see the comment about that the transform-modules-commonjs that will in the future get a spec mode. I guess though that when adding the spec mode to the transform-modules-commonjs plugin it will at least get a minior version bump. For me at least, it makes sense that the transform-modules-commonjs plugin gets the spec option if specified in the preset-env options. If someone runs into problems with this, there is always the option to set modules to false and include transform-modules-commonjs manually without the spec option. @JLHwung @nicolo-ribaudo What are your thoughts on this?

[JLHwung]

I agree that we pass spec and useBuiltins to getPlugin("proposal-dynamic-import"), so that

plugins.push([getPlugin("proposal-dynamic-import"), { loose }]);

could be simplified to

defaultIncludes.push("proposal-dynamic-import");
// generate transformations
transformations = filterItems( ..., defaultIncludes, ... )
// reuse the logic to construct plugins array from transformations.
// transformations => plugins

Passing spec and useBuiltIns should not break transform-modules-* and syntax-dynamic-import. Instead I think they should treat them as reserved option field and implement it when necessary.

[AdamRamberg]

Created a new commit based on @JLHwung suggestions. I also did some refactoring in order to make the code easier to read and follow.

The following changes were made:

• Module plugins are now filtered in the filter-items.js function using the defaultIncludes parameter. See discussion above.
• Did break out the selection of module plugins and created a new function called getModulesPluginNames. Called before we make the call to filter-items.js.
• Did break out the selection of polyfill plugins and created a new function called getPolyfillPlugins. This was mainly done to reduce the length of the main function and make it more readable. Note that polyfills uses filter-items.js internally and these plugins/polyfills needs to be added separately atm. Otherwise we have to make a full rewrite of how polyfills work in preset-env. In my mind this is fine for now.
• Added tests for getModulesPluginNames and getPolyfillPlugins.
• Updated stdout.txt for all debug-fixtures test in babel-preset-env. This had to be done because after this change, when debugging, the modules plugins are logged under ”Using plugins: ”, which makes sense.

Further notes:
I used a destuctured object as parameter to both functions created that increased the number of lines in index.js, but think it improves readability. Not sure if this goes against any code pattern practices in babel-preset-env. I could adjust the commit to use "regular parameters” instead if preferable.

FYI. Screwed up when trying to merge changes from the upstream master. Git tree looks good now though.

[JLHwung]

nit: I am upset by false.toString() can be passed into moduleTransformations, though practically it does not change anything.

I prefer to pass moduleTransformation and modules instead and do the logic inside getModulesPluginNames.

[AdamRamberg]

Not ideal I know! This is one of those situations where using flow makes things less elegant (my own opinion ™).

Made the change you proposed in my latest commit. Used the variable name transformations instead of moduleTransformations inside of getModulesPluginNames so that it not gets confused with the moduleTransformations variable in the global scope.

[nicolo-ribaudo]

Did this throw before this PR? If it didn't, I would prefer not to throw but to warn.

[AdamRamberg]

Before the change it didn't throw, but index.js / filter-items.js could potentially have added a module plugin twice. For example the config that I specified above did that. Users were also able to specify an ambiguous config with for example modules set to cjs and includes set to ['@babel/plugin-transform-modules-amd']. Is it not better if preset-env throws in these cases in order to prevent wonky configs? I believe that there is otherwise a risk that users will overlook the warning. By throwing we are also preventing unnecessary support / issues further down the road for configs behaving weird due to inclusion of module plugins.

[AdamRamberg]

CI is failing after latest review commit. I need to update my branch with the latest commits from master (git pull --rebase upstream master) + commit adjustments to the tests similar to what I did in ae1f9fc in order to get the tests through. After that I will need to force push the changes and my previous commits will have changed hashes, which will (I will imagine) remove / hide the review comments above. Being new to working with PRs and contributing to open source, how would you prefer me to update this PR?

[nicolo-ribaudo]

It's ok to push --force

[AdamRamberg]

It's ok to push --force

Thanks! And the comments are still there 🎉