Navigation Menu

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 plugin-transform-block-scoping const violations #13248

Merged
merged 5 commits into from May 3, 2021

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented May 2, 2021

Fixes #13245.

Q                       A
Fixed Issues? Fixes #13245
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link n/a
Any Dependency Changes? No
License MIT

Fix for #13245.

This PR fixes:

  1. The simple case const c = 1; c = y++;
  2. Assignment expressions const c = 1; c += y++
  3. Update expressions const c = 1; c++
  4. An unrelated bug with for in - see addition to no-for-in/exec.js test for what was previously failing

The tests are maybe written a bit weirdly. My intent was to keep const declarations in same function scope as they were previously, in case that's part of the purpose of the tests. It's a pain testing side effects of code that throws.

I'm leaving the bugs with destructuring for someone else! When/if this PR is merged, I'll open a separate issue for that specifically.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 2, 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 808a0e6:

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

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label May 2, 2021
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.

Thanks!

Comment on lines 458 to 462
t.binaryExpression(
violation.node.operator[0],
violation.get("argument").node,
t.numericLiteral(1),
),
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify this to t.unaryExpression("+", violation.get("argument").node) (even if it saves literally a single byte).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, +c has same effect of triggering .valueOf() as c + 1. Have added a commit to make this change.

I hadn't really bothered with optimization since in most cases code like this is a bug anyway.

Have also added another commit for the other most obvious optimization - shorten c = f() to just f() since the assignment will never be executed anyway. There are other optimizations which could be made (a += 'long_string' to a + '' for example), but in my view going further would add complexity for little gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one more commit to shorten c &&= x++ -> c && x++.

@JLHwung JLHwung merged commit f166b7a into babel:main May 3, 2021
@overlookmotel
Copy link
Contributor Author

Thanks for merging.

@overlookmotel overlookmotel deleted the block-scope-const-violations branch May 3, 2021 17:40
@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels May 5, 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 Aug 5, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: plugin-transform-block-scoping constant checks produce different behaviour
3 participants