Skip to content
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: hoist variable declaration within do block #13122

Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 8, 2021

Q                       A
Fixed Issues? Babel does not hoist variable declaration within do block to the containing function scope, as required by the spec
Patch: Bug Fix? Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

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.

(() => doBlockStatements)()

In this PR we invoke hoistVariables from doBlockStatements, because now the scope of VariableDeclaration is not the target scope we want it to be hoisted to, we should use the hoistVariables helper instead.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Do Expressions labels Apr 8, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 8, 2021

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:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung JLHwung force-pushed the fix-do-expression-block-var-declaration-hoisting branch from 5ecd0f7 to 1a46159 Compare April 8, 2021 14:39
@babel-bot
Copy link
Collaborator

babel-bot commented Apr 8, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46123/

var bar = "foo";
bar;
}
).toBe("foo");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sosukesuzuki I don't like what Prettier is doing here 😛

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What formatting is expected?

Copy link
Member

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an input/output test?

"include": ["./src/**/*"],
"references": [
{
"path": "../babel-types"
Copy link
Contributor Author

@JLHwung JLHwung Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

/cc @nicolo-ribaudo @zxbodya

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

@nicolo-ribaudo
Copy link
Member

@JLHwung The tsconfig.json file shouldn't be necessary anymore after rebasing.

@JLHwung JLHwung force-pushed the fix-do-expression-block-var-declaration-hoisting branch from 17273da to 332569e Compare May 12, 2021 18:41
@nicolo-ribaudo nicolo-ribaudo added this to To review in Nicolò's ideal PR review order list via automation May 31, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Do Expressions
Development

Successfully merging this pull request may close these issues.

None yet

6 participants