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

Jump to bottom

Fix tdz checks in transform-block-scoping plugin #9498

Merged
merged 10 commits into from Jul 21, 2019
Merged

Fix tdz checks in transform-block-scoping plugin #9498

merged 10 commits into from Jul 21, 2019

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 11, 2019 •
edited

Q                       A
Fixed Issues? Fixes #6848, fixes #3707, fixes #3708, fixes #9394
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

The first two commits in this PR are at #9492; I suggest reviewing that PR first.

@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories pkg: traverse labels Feb 11, 2019
nicolo-ribaudo
nicolo-ribaudo commented Feb 11, 2019
View reviewed changes
packages/babel-plugin-transform-block-scoping/src/tdz.js
return "outside";
} else if (executionStatus === "after") {
return "inside";
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that "inside" and "outside" were swapped? To me "inside" means "inside the temporal dead zone", so "the lexical declaration is after the reference".

Copy link
Member

Choose a reason for hiding this comment

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

Yeah looking at the code it seems that "inside" was meant as "inside valid Zone". It is easier to understand for me now after swapping.

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 12, 2019 •
edited

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

@babel-bot
Copy link
Collaborator

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

@danez danez self-requested a review February 18, 2019 23:51
danez
danez approved these changes Mar 1, 2019
View reviewed changes
Copy link
Member

@danez danez left a comment •
edited

Choose a reason for hiding this comment

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

This is some crazy logic to figure out the execution status, but I think I kind of got i partially.

I looked through the tests and all the cases make sense to me.

Just left some nits.

packages/babel-plugin-transform-block-scoping/src/tdz.js
return "outside";
} else if (executionStatus === "after") {
return "inside";
Copy link
Member

Choose a reason for hiding this comment

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

Yeah looking at the code it seems that "inside" was meant as "inside valid Zone". It is easier to understand for me now after swapping.

packages/babel-traverse/src/path/introspection.js Outdated Show resolved Hide resolved
packages/babel-traverse/src/path/introspection.js Outdated Show resolved Hide resolved
@danez
Copy link
Member

danez commented Mar 1, 2019 •
edited

Is this relevant to #7933?

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Mar 3, 2019 •
edited

That issue isn't fixed by this PR.

nicolo-ribaudo
nicolo-ribaudo commented Jul 15, 2019
View reviewed changes
Copy link
Member Author

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

✔️

(I swear I reviewed this PR as if I wasn't the author; it's so old that I didn't remember about a single line of the code I wrote 😂)

@nicolo-ribaudo nicolo-ribaudo merged commit fced5ce into babel:master Jul 21, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the fix-tdz branch July 21, 2019 04:34
This was referenced Sep 15, 2019
Merged
Merged
This was referenced Sep 15, 2019
Merged
Merged
Merged
Closed
Closed
This was referenced Sep 23, 2019
Open
Open
Open
This was referenced Oct 2, 2019
Closed
Closed
Closed
Closed
Open
Open
Open
Closed
Closed
@snyk-bot snyk-bot mentioned this pull request Oct 10, 2019
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jridgewell jridgewell jridgewell approved these changes

@danez danez danez approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
4 participants
@nicolo-ribaudo @babel-bot @danez @jridgewell