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
fix: accept duplicated import/variable in different module #13527
Conversation
both ts and flow fix babel#13510
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4f1c802:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47208/ |
// import declaration can be block scoped when it's in TS/flow module declaration | ||
isImportDeclaration(node) |
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.
Actually it is not block scoped as ImportDeclaration
s may only appear in the top level or inside a module/namespace (for TS). Adding it here may be a breaking change for someone using the validator.
Furthermore excluding ImportDeclaration
s from
babel/packages/babel-traverse/src/scope/index.ts
Lines 201 to 212 in fce35af
Declaration(path) { | |
// delegate block scope handling to the `BlockScoped` method | |
if (path.isBlockScoped()) return; | |
// this will be hit again once we traverse into it after this iteration | |
if (path.isExportDeclaration()) return; | |
// we've ran into a declaration! | |
const parent = | |
path.scope.getFunctionParent() || path.scope.getProgramParent(); | |
parent.registerDeclaration(path); | |
}, |
results in excluding those nodes from the traversal and not running
registerDeclaration
on them.
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.
I don't think excluding ImportDeclarations
is well enough, since it will also cause breaking changes.
The main problem here is that the scope of declared module should be isolated with other modules' scopes and the program's scope. But unfortunately it's not now.
So maybe we could change the implementation of getProgramParent
to make it returns not only on Program
, but also ModuleDeclaration
. This will make scopes of modules isolated with each other and the program.
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.
Yep, excluding ImportDeclaration
is definitely not the right way here.
You will be happy to know that there is already a method called getBlockParent
that gets the upper block. However I would suggest you to give a look at how the BlockScoped
visitor is handling block scoped declarations, because we may have to replicate a similar behavior for ImportDeclaration
.
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.
make it returns not only on Program, but also ModuleDeclaration.
I don't think we can mark ModuleDeclaration as program because variables within module declaration can reference bindings defined in program:
type T = number;
declare module 'test' {
declare var a: T;
}
Since ambient declarations are always top level. We can check if node.parent
is ModuleDeclaration
and then register declarations there, otherwise we fallback to
babel/packages/babel-traverse/src/scope/index.ts
Lines 209 to 210 in fce35af
const parent = | |
path.scope.getFunctionParent() || path.scope.getProgramParent(); |
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.
I took a different approach.
Take all ImportDeclaration
into a separated visitor, and use getBlockParent
to find parent scope.
I think it is more conducive to maintenance。
restore isBlockScoped validator
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
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.
Thanks!
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
babel/packages/babel-traverse/src/scope/index.ts
Lines 201 to 212 in fce35af
I make all
ImportDeclaration
isBlockScoped
, so that it willgetBlockParent
instead ofgetProgramParent
(which leads to conflicts).I'm not sure this is the best way to do so, but since
Declaration
andBlockScoped
visitor ofcollectorVisitor
are doing similar things, it sounds like pretty good to me.According to the test, this change did not bring any breaking change. However, I not so familiar with babel codebase. So there are possibilities that this PR is breaking something.
Let me know if this approach is not correct / breaking, I will try to find another way out.