Refine reexport test cases - #1947
Conversation
lukastaegert
left a comment
There was a problem hiding this comment.
Very nice! Hope to get this into the next release!
| if (exportDeclaration) { | ||
| // if export binding is itself an import binding then continue tracing | ||
| const importDeclaration = module.imports[exportDeclaration.localName]; | ||
| if (importDeclaration) { |
There was a problem hiding this comment.
Minor nit: As this introduces a lot of special cases and return statements that break the even abstraction layer of the traceExport function and makes it harder to reason about, I would suggest extracting the body of this if-statement into a separate private method so that the code in traceExport could become e.g.
if (exportDeclaration) {
const importDeclaration = module.imports[exportDeclaration.localName];
if (importDeclaration) {
return this.traceReexportedImport(module, importDeclaration);
}
return { name, module };
}| const importDeclaration = module.imports[exportDeclaration.localName]; | ||
| if (importDeclaration) { | ||
| // add the import binding to the shape | ||
| const declaration = module.imports[exportDeclaration.localName]; |
There was a problem hiding this comment.
This variable is basically importDeclaration
There was a problem hiding this comment.
Also, the comment preceding this line does not really seem to describe this line but something that happens further down, which is confusing. Actually once this function is extracted, I think the code is understandable even with fewer comments, and comments do not tend to survive refactorings well anyway.
|
|
||
| const exportsNames = imports && imports.some(specifier => specifier.imported !== 'default' && specifier.imported !== '*') || | ||
| reexports && reexports.some(specifier => specifier.imported === 'default'); | ||
| reexports && reexports.some(specifier => specifier.imported !== 'default' && specifier.imported !== '*'); |
| @@ -1,5 +1,2 @@ | |||
| import x from 'x'; | |||
There was a problem hiding this comment.
I guess its not perfect that this line is retained but probably unavoidable as we are not counting usages.
There was a problem hiding this comment.
Yes exactly, if we could track per-chunk usage then this can change, but until then the import statements are never pruned. These statements are well-merged though so it typically doesn't result in any bloat.
| }); | ||
|
|
||
| dependencies.forEach(({ name, reexports }) => { | ||
| dependencies.forEach(({ name, imports, reexports }) => { |
There was a problem hiding this comment.
I know these two loops have previously been separate but it seems they can easily be combined into one loop which will remove one iteration + remove one if statement + make sure in a hard-coded way no reexport is handled twice.
There was a problem hiding this comment.
The reason for this separation is to ensure that the star exports come first in the output, which is important to the export ordering as star exports get overridden by reexports get overridden by exact name exports.
| if (imports) | ||
| output += '\n'; | ||
| const starExport = reexports.find(specifier => specifier.reexported === '*'); | ||
| const nsReexport = reexports.find(specifier => specifier.imported === '*' && specifier.reexported !== '*'); |
There was a problem hiding this comment.
I would love a longer name here, e.g. nonStarReexport as with the shortened name, you have no chance of knowing what it means further down without reading the declaration.
| } | ||
|
|
||
| this.populateImport(variable, tracedExport); | ||
| this.traceImport(declaration.module, declaration.name); |
There was a problem hiding this comment.
Ah, I totally overlooked this one. So extraction actually removed a code duplication 👍
This refines some more cases around reexport boundaries, specifically supporting #1675 case of reexporting an imported external namespace, and also further cases around tracing
import { x } export { x }scenarios identically to other re-exports in the boundaries.It feels like we're finally getting close to complete on these scenarios, although there may still be a few surprises to discover.
Edit: Also fixed #1951