Default and export naming fixes - #2001
Conversation
|
@guybedford Tried this out over in mapbox/mapbox-gl-js#6196 and I'm seeing: exports.* = commonjsGlobal;
exports.*$1 = unwrapExports;
exports.*$2 = createCommonjsModule;in chunk1.js |
| super(name, exportDefaultDeclaration, exportDefaultDeclaration.declaration); | ||
| this.isDefault = true; | ||
| this.hasId = !!(<FunctionDeclaration | ClassDeclaration>exportDefaultDeclaration.declaration).id; | ||
| this.exportName = 'default'; |
There was a problem hiding this comment.
How hard would it be to include the filename of the module as part of this name, e.g., my_module$default for the default export of my-module.js? Seems like this could make for easier debugging in some cases.
There was a problem hiding this comment.
The reason we use the initial name is because this stage of the algorithm is still attempting to determine if the chunk is an entry point module itself (entry.js), or if it is a chunk (chunk[i].js). Ideally the algorithm could do the export naming after this stage in the process in which case we can better handle naming definitely. I will look into a refactor along these lines, but perhaps not just yet.
There was a problem hiding this comment.
(So basically in the scenario that the chunk is an entry point, we need it to do the correct naming as the entry point would expect)
diff --git a/src/Chunk.ts b/src/Chunk.ts
index 6c01905c..69b6eea4 100644
--- a/src/Chunk.ts
+++ b/src/Chunk.ts
@@ -146,9 +146,10 @@ export default class Chunk {
}
let i = 0;
- safeExportName = exportName;
+ const baseExportName = exportName === '*' ? '_namespace' : exportName;
+ safeExportName = baseExportName;
while (this.exports[safeExportName]) {
- safeExportName = exportName + '$' + ++i;
+ safeExportName = baseExportName + '$' + ++i;
}
variable.exportName = safeExportName;Fixes the above for me |
|
@anandthakker any chance of a test case? I take it you have a namespace import across a chunk boundary? |
|
@guybedford y, I'll try to reduce a test case. We don't have an explicit namespace import -- from the variable names ( |
|
Sure no stress, I'll run it on my test case here for that too and see what I can find. |
|
@guybedford reduced test case here: https://gist.github.com/anandthakker/68b1b63f2c1236e85709aae0f9f80ad9 |
|
@anandthakker amazing thanks for isolating - my CJS case here didn't catch it. |
|
@anandthakker I've included the fix here. Thanks again for the test cases. |
|
@guybedford thanks! Confirmed that it's working in practice over at https://github.com/mapbox/mapbox-gl-js/tree/6fa5becea62ac5a58a5eb1572fb764c64e50fa36 |
|
(By the way, @guybedford, you can also see there the hacky way we're using code splitting to approximate the |
| this.entryModule = undefined; | ||
| this.isEntryModuleFacade = false; | ||
| if (orderedModules.length === 0) | ||
| this.isEntryModuleFacade = true; |
There was a problem hiding this comment.
This would easily merge with the previous assignment.
| ensureExport (module: Module | ExternalModule, variable: Variable): string { | ||
| ensureExport (module: Module | ExternalModule, variable: Variable, exportName: string): string { | ||
| // assert(module.chunk === this || module.isExternal); | ||
| let safeExportName = this.exportedVariables.get(variable); |
There was a problem hiding this comment.
Maybe we should actually call this this.exportedVariableNames to make clear what we are mapping the variables to.
| } | ||
|
|
||
| return safeExportName; | ||
| } |
|
|
||
| // we can disregard exports.foo etc | ||
| if (declaration.exportName && declaration.isReassigned && !declaration.isId) | ||
| if (declaration.exportName && (declaration.isDefault || declaration.isReassigned && !declaration.isId)) |
There was a problem hiding this comment.
The non-use of more parentheses here makes me a little uneasy (sorry, but I'm one of those people who need to look into an operator precedence table to see that && binds more strongly than ||).
lukastaegert
left a comment
There was a problem hiding this comment.
Awesome! I also very much like the additional private modifiers that will help us keep the API surface small.
|
I'm trying to test this branch out, but I can't quite get npm to install it - can anyone here possibly lend a hand? As far as I can tell, this is what I want
but npm doesn't seem to like it (nor plenty of the variations on that theme which I've also tried) This seems to fail too :(
|
|
Hi @arackaf, you should probably check your npm version. As rollup relies on its Nevertheless, I am about to release this branch now anyway 😉 |
|
Aha! Thanks Lukas. I'm still too scared to go back in the npm 5 waters. I reverted back to the comfort and safety of npm 4 since many of the 5 bugs affected me directly. |
|
@arackaf for future reference try |
|
@arackaf Might be worth giving npm@5 another shot. A lot has been fixed and improved since 5.0.0. We're using it at work for two large projects and after some initial hickups, it's now been smooth riding for quite a few months now. The performance benefits are nothing to sneeze at 😉 |
This fixes #1996 along with some other cases around export default and named exports:
safeNameand doesn't get a safe name when there's an original reassignment.defaultby default, while we were using their import aliases before.The depths of these cases keeps expanding... I'm sure it will continue to! But as long as we put equal effort into refactoring and fixes we should be on a good footing (and project sponsorship has been a huge help to justify this time input!!).