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 update expression for exported bigints #14341

Merged
merged 6 commits into from Mar 10, 2022
Merged

Fix update expression for exported bigints #14341

merged 6 commits into from Mar 10, 2022

Conversation

magic-akari
Copy link
Contributor

@magic-akari magic-akari commented Mar 9, 2022

Q                       A
Fixed Issues? Fixes #14340
Patch: Bug Fix? 👍
Major: Breaking Change? behavior of @babel/helper-simple-access changed
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@nicolo-ribaudo nicolo-ribaudo added area: modules PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: BigInt labels Mar 9, 2022
@@ -20,73 +16,6 @@ export default function simplifyAccess(path: NodePath, bindingNames) {
}

const simpleAssignmentVisitor = {
UpdateExpression: {
Copy link
Member

Choose a reason for hiding this comment

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

If someone accidentally updates @babel/helper-simple-access without also updating @babel/helper-module-transforms (this is a valid thing to do, according to the semver dependency versions), I fear that this will break their current code.

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.
What can I do? Should I bump the version of the package?

Copy link
Member

Choose a reason for hiding this comment

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

No, because we keep all the versions synchronized (and it would require a major bump).

It's annoying, but a solution until Babel 8 is to pass a includeUpdateExpression: boolean = true option to simplifyAccess, and only skip the UpdateExpression visitor when that option is set to false.

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.

for the new tests

@nicolo-ribaudo nicolo-ribaudo changed the title Fix update expression for bigints Fix update expression for exported bigints Mar 10, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit a7b1181 into babel:main Mar 10, 2022
@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 Jun 10, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: modules outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: BigInt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: update Expression transform into wrong result in cjs
3 participants