System module format hoisting and export refinements #8104
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8272/ |
There was a problem hiding this comment.
Hoisting
As discussed with @loganfsmyth, the goal of hoisting is to support ES module semantics through the system format.Any code that runs correctly as an ES module, should run correctly through the System format.
What is an example of ES code which doesn't work if we don't hoist const/let in the SystemJS output? 🤔
| @@ -29,7 +29,7 @@ const visitor = { | |||
| } | |||
|
|
|||
| for (const name in declar.getBindingIdentifiers()) { | |||
| state.emit(t.identifier(name), name); | |||
| state.emit(t.identifier(name), name, declar.node.init !== null); | |||
There was a problem hiding this comment.
Init could also be undefined, because it is an optional node. Maybe !!declar.node.init?
There was a problem hiding this comment.
According to the ESTree this should be null... didn't know undefined was permitted here?
There was a problem hiding this comment.
Whops you are right, it defaults to nul.
Circular references with function exports. See https://github.com/ModuleLoader/es-module-loader/blob/master/docs/system-register.md for more info. |
hzoo
added
the
PR: Bug Fix 🐛
|
I'm not sure if I'm using this correctly, but I had to revert to babel-plugin-transform-es2015-modules-systemjs as I was losing access to exported functions. My test repo is here . If you install and run gulp the output scripts in the build dir show the old transform vs the new, and in the new it seems my script's testFunc named export isn't available. |
|
Could you open a new issue? I don't have to check if it is a bug right now, but it's easy to forget about it if it is only reported in a comment of a closed PR 🙏 |
|
@sgilligan thanks for noting this - yes it would have been appropriate for a bug report. The test case was perfect for reproducing the issue and tracking it down in #8820. |
|
Hi gents, sorry for not filing as a separate bug report, @guybedford thanks for taking care of it. Cheers. |
lock
bot
added
the
outdated
This PR fixes up some issues with hoisting, destructuring assignment and export calls.
Please just let me know if there are any questions here at all, and happy to integrate any feedback.
Hoisting
As discussed with @loganfsmyth, the goal of hoisting is to support ES module semantics through the system format.
Any code that runs correctly as an ES module, should run correctly through the System format.
As a result, we should always hoist, and not opt out of hoisting when hitting up on let or const declarations.
This does mean we don't catch TDZ errors, but the assumption here is that a working module graph for working code shouldn't need to rely on TDZ errors for proper function in the normal running case.
Destructuring assignments
Support has been added for cases like
export var p = 5; ({ p } = q);where previously, the destructured bindings weren't being checked on the assignment expression update.Pruning unnecessary export calls
There seem to have been some unnecessary export calls that crept in as well with the destructuring updates, as
export var p = 5would output two export calls, one for the binding definition, and another for the assignment expression when hoisted. This is fixed by only emitting for the assignment expression and always hoisting.Ensuring export calls for uninitialized bindings
To handle the
export var pcase without initialization, ahasInitreturn has been added to the hoisting helper function as a third argument, which whenfalseinforms the transform that an uninitialized_export("p", undefined)is needed at the declaration phase. This also ensures we consistently export for each export name of the module guaranteeing the module object will have the right shape by the end of execution time.Combining reexport calls
Reexports of the form
export { p, q } from 'asdf'would create the anvar exportObj = {}in the setter, and then assign each export to that. I've separated this process out into a separateconstructExportCallfunction which has three cases - single export, multiple exports, and multiple exports with export star. The single export case then becomes_export("p", asdf.p), and the multiple exports case can become_export({ p: asdf.p, q: asdf.q })making this a bit cleaner.