-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
@@ -14,6 +14,7 @@ export default class ExportDefaultVariable extends LocalVariable { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
src/Chunk.ts
Outdated
@@ -115,6 +115,8 @@ export default class Chunk { | |||
this.dependencies = undefined; | |||
this.entryModule = undefined; | |||
this.isEntryModuleFacade = false; | |||
if (orderedModules.length === 0) | |||
this.isEntryModuleFacade = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would easily merge with the previous assignment.
src/Chunk.ts
Outdated
@@ -136,44 +138,30 @@ export default class Chunk { | |||
// ensure that the module exports or reexports the given variable | |||
// we don't replace reexports with the direct reexport from the final module | |||
// as this might result in exposing an internal module which taints an entryModule chunk | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should actually call this this.exportedVariableNames
to make clear what we are mapping the variables to.
else { | ||
curExport.name = safeExportName; | ||
} | ||
|
||
return safeExportName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/ast/scopes/Scope.ts
Outdated
@@ -86,7 +86,7 @@ export default class Scope { | |||
const declaration = this.variables[key]; | |||
|
|||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ||
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
safeName
and doesn't get a safe name when there's an original reassignment.default
by 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!!).