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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
17 changes: 17 additions & 0 deletions
17
...el-traverse/test/fixtures/regression/duplicate-variable-in-different-module-flow/input.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// @flow | ||
|
||
declare module 'test' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
declare var a: number; | ||
declare function foo(a: JSONSchema7): string; | ||
declare export function concatPath(dirA: string, dirB: string): string; | ||
declare export class A {} | ||
} | ||
|
||
declare module 'test/submodule' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
declare var a: number; | ||
declare function foo(a: JSONSchema7): string; | ||
declare export function concatPath(dirA: string, dirB: string): string; | ||
declare export class A {} | ||
} |
3 changes: 3 additions & 0 deletions
3
...raverse/test/fixtures/regression/duplicate-variable-in-different-module-flow/options.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"plugins": ["syntax-flow"] | ||
} |
15 changes: 15 additions & 0 deletions
15
...l-traverse/test/fixtures/regression/duplicate-variable-in-different-module-flow/output.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// @flow | ||
declare module 'test' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
declare var a: number; | ||
declare function foo(a: JSONSchema7): string; | ||
declare export function concatPath(dirA: string, dirB: string): string; | ||
declare export class A {} | ||
} | ||
declare module 'test/submodule' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
declare var a: number; | ||
declare function foo(a: JSONSchema7): string; | ||
declare export function concatPath(dirA: string, dirB: string): string; | ||
declare export class A {} | ||
} |
13 changes: 13 additions & 0 deletions
13
...abel-traverse/test/fixtures/regression/duplicate-variable-in-different-module-ts/input.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
declare module 'test' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
import { bar } from "baz"; | ||
export { fooBar } from "baz"; | ||
let foo: JSONSchema7; | ||
} | ||
|
||
declare module 'test/submodule' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
import { bar } from "baz"; | ||
export { fooBar } from "baz"; | ||
let foo: JSONSchema7; | ||
} |
4 changes: 4 additions & 0 deletions
4
...-traverse/test/fixtures/regression/duplicate-variable-in-different-module-ts/options.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"plugins": ["syntax-typescript"], | ||
"sourceType": "module" | ||
} |
12 changes: 12 additions & 0 deletions
12
...el-traverse/test/fixtures/regression/duplicate-variable-in-different-module-ts/output.mjs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
declare module 'test' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
import { bar } from "baz"; | ||
export { fooBar } from "baz"; | ||
let foo: JSONSchema7; | ||
} | ||
declare module 'test/submodule' { | ||
import type { JSONSchema7 } from 'json-schema'; | ||
import { bar } from "baz"; | ||
export { fooBar } from "baz"; | ||
let foo: JSONSchema7; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,20 @@ | ||
import { isClassDeclaration, isFunctionDeclaration } from "./generated"; | ||
import { | ||
isClassDeclaration, | ||
isFunctionDeclaration, | ||
isImportDeclaration, | ||
} from "./generated"; | ||
import isLet from "./isLet"; | ||
import type * as t from ".."; | ||
|
||
/** | ||
* Check if the input `node` is block scoped. | ||
*/ | ||
export default function isBlockScoped(node: t.Node): boolean { | ||
return isFunctionDeclaration(node) || isClassDeclaration(node) || isLet(node); | ||
return ( | ||
isFunctionDeclaration(node) || | ||
isClassDeclaration(node) || | ||
isLet(node) || | ||
// import declaration can be block scoped when it's in TS/flow module declaration | ||
isImportDeclaration(node) | ||
); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 frombabel/packages/babel-traverse/src/scope/index.ts
Lines 201 to 212 in fce35af
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 onProgram
, but alsoModuleDeclaration
. 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 theBlockScoped
visitor is handling block scoped declarations, because we may have to replicate a similar behavior forImportDeclaration
.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 we can mark ModuleDeclaration as program because variables within module declaration can reference bindings defined in program:
Since ambient declarations are always top level. We can check if
node.parent
isModuleDeclaration
and then register declarations there, otherwise we fallback tobabel/packages/babel-traverse/src/scope/index.ts
Lines 209 to 210 in fce35af
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 usegetBlockParent
to find parent scope.I think it is more conducive to maintenance。