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
[systemjs] Fix nested let
/const
shadowing imported bindings
#14057
Changes from 7 commits
45f4c1f
8199f31
69c067b
a312fe2
15714de
6fe46d8
14e9cfd
aa5593e
aeb81ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,9 @@ import { template, types as t } from "@babel/core"; | |
import { getImportSource } from "babel-plugin-dynamic-import-node/utils"; | ||
import { rewriteThis, getModuleName } from "@babel/helper-module-transforms"; | ||
import { isIdentifierName } from "@babel/helper-validator-identifier"; | ||
import type { NodePath } from "@babel/traverse"; | ||
|
||
const buildTemplate = template(` | ||
const buildTemplate = template.statement(` | ||
SYSTEM_REGISTER(MODULE_NAME, SOURCES, function (EXPORT_IDENTIFIER, CONTEXT_IDENTIFIER) { | ||
"use strict"; | ||
BEFORE_BODY; | ||
|
@@ -16,7 +17,7 @@ const buildTemplate = template(` | |
}); | ||
`); | ||
|
||
const buildExportAll = template(` | ||
const buildExportAll = template.statement(` | ||
for (var KEY in TARGET) { | ||
if (KEY !== "default" && KEY !== "__esModule") EXPORT_OBJ[KEY] = TARGET[KEY]; | ||
} | ||
|
@@ -286,7 +287,7 @@ export default declare((api, options) => { | |
rewriteThis(path); | ||
} | ||
}, | ||
exit(path, state: PluginState) { | ||
exit(path: NodePath<t.Program>, state: PluginState) { | ||
const scope = path.scope; | ||
const exportIdent = scope.generateUid("export"); | ||
const { contextIdent, stringSpecifiers } = state; | ||
|
@@ -332,7 +333,7 @@ export default declare((api, options) => { | |
const exportNames = []; | ||
const exportValues = []; | ||
|
||
const body: Array<any> = path.get("body"); | ||
const body = path.get("body"); | ||
|
||
for (const path of body) { | ||
if (path.isFunctionDeclaration()) { | ||
|
@@ -349,6 +350,10 @@ export default declare((api, options) => { | |
), | ||
), | ||
); | ||
} else if (path.isVariableDeclaration()) { | ||
// Convert top-level variable declarations to "var", | ||
// because they must be hoisted | ||
path.node.kind = "var"; | ||
} else if (path.isImportDeclaration()) { | ||
const source = path.node.source.value; | ||
pushModule(source, "imports", path.node.specifiers); | ||
|
@@ -362,8 +367,8 @@ export default declare((api, options) => { | |
path.remove(); | ||
} else if (path.isExportDefaultDeclaration()) { | ||
const declar = path.get("declaration"); | ||
const id = declar.node.id; | ||
if (declar.isClassDeclaration()) { | ||
const id = declar.node.id; | ||
if (id) { | ||
exportNames.push("default"); | ||
exportValues.push(scope.buildUndefinedNode()); | ||
|
@@ -384,6 +389,7 @@ export default declare((api, options) => { | |
removedPaths.push(path); | ||
} | ||
} else if (declar.isFunctionDeclaration()) { | ||
const id = declar.node.id; | ||
if (id) { | ||
beforeBody.push(declar.node); | ||
exportNames.push("default"); | ||
|
@@ -403,15 +409,15 @@ export default declare((api, options) => { | |
if (declar.node) { | ||
path.replaceWith(declar); | ||
|
||
if (path.isFunction()) { | ||
if (declar.isFunction()) { | ||
const node = declar.node; | ||
const name = node.id.name; | ||
addExportName(name, name); | ||
beforeBody.push(node); | ||
exportNames.push(name); | ||
exportValues.push(t.cloneNode(node.id)); | ||
removedPaths.push(path); | ||
} else if (path.isClass()) { | ||
} else if (declar.isClass()) { | ||
const name = declar.node.id.name; | ||
exportNames.push(name); | ||
exportValues.push(scope.buildUndefinedNode()); | ||
|
@@ -427,6 +433,11 @@ export default declare((api, options) => { | |
); | ||
addExportName(name, name); | ||
} else { | ||
if (declar.isVariableDeclaration()) { | ||
// Convert top-level variable declarations to "var", | ||
// because they must be hoisted | ||
declar.node.kind = "var"; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A top level var declaration can be nested within for statement: for (var x = 1;false;);
export { x } or it could be within a do expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They already have |
||
for (const name of Object.keys( | ||
declar.getBindingIdentifiers(), | ||
)) { | ||
|
@@ -443,7 +454,10 @@ export default declare((api, options) => { | |
const nodes = []; | ||
|
||
for (const specifier of specifiers) { | ||
// @ts-expect-error This isn't an "export ... from" declaration | ||
// because path.node.source is falsy, so the local specifier exists. | ||
const { local, exported } = specifier; | ||
|
||
const binding = scope.getBinding(local.name); | ||
const exportedName = getExportSpecifierName( | ||
exported, | ||
|
@@ -565,19 +579,15 @@ export default declare((api, options) => { | |
// @ts-expect-error todo(flow->ts): do not reuse variables | ||
if (moduleName) moduleName = t.stringLiteral(moduleName); | ||
|
||
hoistVariables( | ||
path, | ||
(id, name, hasInit) => { | ||
variableIds.push(id); | ||
if (!hasInit && name in exportMap) { | ||
for (const exported of exportMap[name]) { | ||
exportNames.push(exported); | ||
exportValues.push(scope.buildUndefinedNode()); | ||
} | ||
hoistVariables(path, (id, name, hasInit) => { | ||
variableIds.push(id); | ||
if (!hasInit && name in exportMap) { | ||
for (const exported of exportMap[name]) { | ||
exportNames.push(exported); | ||
exportValues.push(scope.buildUndefinedNode()); | ||
} | ||
}, | ||
null, | ||
); | ||
} | ||
}); | ||
|
||
if (variableIds.length) { | ||
beforeBody.unshift( | ||
|
@@ -620,6 +630,7 @@ export default declare((api, options) => { | |
Function(path) { | ||
path.skip(); | ||
}, | ||
// @ts-expect-error - todo: add noScope to type definitions | ||
noScope: true, | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { x } from './x.js'; | ||
|
||
if (true) { | ||
const { x } = { x: 1 }; | ||
console.log(x); | ||
} | ||
|
||
new (class extends x {})(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
System.register(["./x.js"], function (_export, _context) { | ||
"use strict"; | ||
|
||
var x; | ||
return { | ||
setters: [function (_xJs) { | ||
x = _xJs.x; | ||
}], | ||
execute: function () { | ||
if (true) { | ||
const { | ||
x | ||
} = { | ||
x: 1 | ||
}; | ||
console.log(x); | ||
} | ||
|
||
new class extends x {}(); | ||
} | ||
}; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { x } from './x.js'; | ||
|
||
if (true) { | ||
const x = 1; | ||
console.log(x); | ||
} | ||
|
||
new (class extends x {})(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
System.register(["./x.js"], function (_export, _context) { | ||
"use strict"; | ||
|
||
var x; | ||
return { | ||
setters: [function (_xJs) { | ||
x = _xJs.x; | ||
}], | ||
execute: function () { | ||
if (true) { | ||
const x = 1; | ||
console.log(x); | ||
} | ||
|
||
new class extends x {}(); | ||
} | ||
}; | ||
}); |
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 hoistVariable visitor should be merged with
environmentVisitor
, too.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.
Lets do it in another PR since it's separate from this bug 👍