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

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

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.

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

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?

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.

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.

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?

It's ok to push --force

It's ok to push --force

Thanks! And the comments are still there 🎉