Refactor handling of modules plugins in preset-env
#15988
Conversation
nicolo-ribaudo
added
the
PR: Internal 🏠
| })); | ||
| const modules = | ||
| optionsModules === "auto" | ||
| ? api.caller(supportsStaticESM) |
There was a problem hiding this comment.
?. removed from api.caller because every Babel 7 version provides api.caller. This was only needed during the beta period.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55665/ |
| @@ -160,15 +157,12 @@ export const getModulesPluginNames = ({ | |||
| if (!shouldTransformDynamicImport) { | |||
| modulesPluginNames.push("syntax-dynamic-import"); | |||
| } | |||
| if (!shouldTransformExportNamespaceFrom) { | |||
| modulesPluginNames.push("syntax-export-namespace-from"); | |||
There was a problem hiding this comment.
This does not need to be included, because this plugin also goes through the normal process for including transform/syntax plugins. The only reason this plugin has special handling is that we must forcefully enable the transform if the targets support it but the caller does not.
Maybe we should get rid of this behavior in Babel 8, similarly to how the caller does not affect any other plugin.
There was a problem hiding this comment.
Can you add transform-export-namespace-from to the pluginSyntaxObject in packages/babel-preset-env/src/shipped-proposals.ts? So that we can enable the syntax plugin if compiling to Chrome 117 on Babel 7.0.0.
| const shouldSkipExportNamespaceFrom = | ||
| (modules === "auto" && api.caller?.(supportsExportNamespaceFrom)) || | ||
| (modules === false && | ||
| !isRequired("transform-export-namespace-from", transformTargets, { |
There was a problem hiding this comment.
No need to check isRequired here, given that it will be also re-checked as for any other plugin if the transform is not forcefully included.
| @@ -71,7 +71,6 @@ describe("babel-preset-env", () => { | |||
| }), | |||
There was a problem hiding this comment.
getModulesPluginNames is a public interface, it may be a breaking change to remove the syntax plugin here. In Babel 7 we have to unconditionally enable syntax-export-namespace-from anyway, I think it is fine to include it in getModulesPluginNames, we can remove those plugins in Babel 8.
There was a problem hiding this comment.
In Babel 8 let's also make this function not public anymore
|
|
||
| // TODO(Babel 8): This is only here for backward compatibility. Remove it. | ||
| export { getModulesPluginNamesBackwardCompat as getModulesPluginNames }; | ||
| const getModulesPluginNamesBackwardCompat = ({ |
There was a problem hiding this comment.
This function is not changed, I just moved it down here and exported it for backward compat. I'll open a PR removing it for Babel 8.
| !exclude.plugins.has("transform-export-namespace-from") | ||
| ) { | ||
| include.plugins.add("transform-export-namespace-from"); | ||
| } |
There was a problem hiding this comment.
@JLHwung Now transform-export-namespace-from is 100% handled as a normal plugin, with this if statement being the only exception.
The other two modules plugins are still "special" because it does make no sense to do targets-based compilation for them given that they completely change the underlying module system, so they are still handled by the separate function.
If you look at the diffs, now the only actual output difference is that we enable syntax-dynamic-import in some cases where we weren't doing so previously.
| bugfix/transform-v8-spread-parameters-in-optional-chaining { chrome < 91 } | ||
| transform-modules-commonjs | ||
| transform-dynamic-import | ||
| transform-export-namespace-from { } | ||
| syntax-import-meta | ||
| syntax-import-attributes |
There was a problem hiding this comment.
These in the debug folder are Babel 8 tests, I'm not sure about why they included the syntax plugins but 🤷 the new output is correct
nicolo-ribaudo
force-pushed
the
env-modules-refactor
branch
from
4ebe30e
to
4acd165
Compare
nicolo-ribaudo
force-pushed
the
env-modules-refactor
branch
from
4acd165
to
34f4e30
Compare
github-actions
bot
added
the
outdated
I was working on preserving ESM by default in Babel 8 (unless the caller explicitly says that it does not support ESM), and I noticed that
transform-export-namespace-fromhad some useless interactions with target checks. While removing it, I also noticed that we don't add the dynamic import syntax plugin in some cases when we are not transpiling it (we should include it for old@babel/parserversions).