fix: hoist variable declaration within do block#13122
Conversation
|
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 fdc152f:
|
5ecd0f7 to
1a46159
Compare
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46123/ |
| var bar = "foo"; | ||
| bar; | ||
| } | ||
| ).toBe("foo"); |
There was a problem hiding this comment.
@sosukesuzuki I don't like what Prettier is doing here 😛
There was a problem hiding this comment.
What formatting is expected?
There was a problem hiding this comment.
I would have expected it to be
expect(do {
var bar = "foo";
bar;
}).toBe("foo");
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Can you add an input/output test?
| "include": ["./src/**/*"], | ||
| "references": [ | ||
| { | ||
| "path": "../babel-types" |
There was a problem hiding this comment.
Currently the tsconfig generator will generate one more item:
{ "path": "../babel-traverse" }I am not familiar with the references usage here but the type checking and the VSCode type inferences seems to work after I removed babel-traverse, which caused cycling dependencies since this package is also depended by @babel/traverse.
Should we filter out dependencies from introduced only from type imports? In this case we have import type { NodePath } from "@babel/traverse".
There was a problem hiding this comment.
The dependency is needed also for type imports: it's needed to tell TS in which order to compile the packages to properly type-check everything.
TS doesn't support circular dependencies yet (microsoft/TypeScript#33685), so we could consider moving to type-checking all the packages as a whole rather than package by package (it will make incremental type-checks a bit slower, but it doesn't have this ordering problem).
There was a problem hiding this comment.
I would suggest, if removing reference to @babel/traverse we should also remove type import:
import type { NodePath } from "@babel/traverse";
Might be using any for now.
Otherwise it might result in some weird errors - something like "can not output to file used as input" (basically compiled TS would be imported here, but when compiling @babel/traverse this file would be used as input, which would mean - when compiling @babel/traverse TS will try to use output of it as an import )
|
@JLHwung The |
17273da to
332569e
Compare
Although we were aware of this issue and intended to hoist variables by
this.traverse(hoistVariablesVisitor), the variable declarations were never hoisted because the visitor stops at the function closure wrapping do blocks.In this PR we invoke
hoistVariablesfromdoBlockStatements, because now the scope of VariableDeclaration is not the target scope we want it to be hoisted to, we should use thehoistVariableshelper instead.