Skip to content

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

Merged
nicolo-ribaudo merged 10 commits intobabel:masterfrom
nicolo-ribaudo:fix-tdz
Jul 21, 2019
Merged

Fix tdz checks in transform-block-scoping plugin#9498
nicolo-ribaudo merged 10 commits intobabel:masterfrom
nicolo-ribaudo:fix-tdz

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

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

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
} else if (executionStatus === "after") {
return "outside";
} else if (executionStatus === "after") {
return "inside";
Copy link
Copy Markdown
Member Author

@nicolo-ribaudo nicolo-ribaudo Feb 11, 2019

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
Copy Markdown
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
Copy Markdown
Collaborator

babel-bot commented Feb 12, 2019

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

@babel-bot
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@danez danez left a comment

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.

} else if (executionStatus === "after") {
return "outside";
} else if (executionStatus === "after") {
return "inside";
Copy link
Copy Markdown
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.

@danez
Copy link
Copy Markdown
Member

danez commented Mar 1, 2019

Is this relevant to #7933?

@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Mar 3, 2019

That issue isn't fixed by this PR.

Copy link
Copy Markdown
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 😂)

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 pkg: traverse PR: Spec Compliance 👓 A type of pull request used for our changelog categories

Projects

None yet

4 participants