Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Duplicate function call in variable destructuring #13711
fix: Duplicate function call in variable destructuring #13711
Changes from 13 commits
51c520d
b74dfb9
cb530b1
698b776
b1c393e
87a02d1
84c2300
3c851b4
af17b56
8ace7f1
ab3b603
80fb872
60cabe8
df3d149
80b1692
317065d
4be2624
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR: I think we should use
originalPath.scope.isStatic(originalPath.node.init)
here, becauseThisExpression
is safe to reuse, e.g.node.init
is an identifier, it can not be reused if not a constant binding. e.g.y
becomes{ v: 2 }
after transformed becausez
can be modified insideEffect()
.These two issues will be fixed by
scope.isStatic
, they can be addressed in a subsequent PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Can you add a test case with mixed arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed up a commit. Thanks for the recommendation, it caught an edge case with the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a new test case?
A RestElement's argument can be a pattern so we have to recurse into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point! I added the test case and confirmed the output looks as I'd expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not return early when
properties
is falsy, instead we should invoke the recursive function on.argument
. For example,has only one binding identifier but
doesAnyChildNodeHaveMoreThanOneProperty
returnstrue
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JLHwung - thanks for the the detailed case. I looked through https://astexplorer.net/ and believe I was able to diagnose the issue you called out with more robust test cases for array destructuring.
Happy to make any other adjustments. Thank you for the thorough review and prompt responses!