-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix each block destructuring can't have default values refer to destructured params #5230
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
Conversation
const n = contexts.length; | ||
|
||
unpack_destructuring(contexts, value.left, node => { | ||
const alternate = JSON.parse(JSON.stringify(value.right)); |
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.
why cloning the node?
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.
because the node will be altered in update_reference
and I cannot get it right without cloning it. I noticed that modifier
will be called several times in the process, if the node changes during the process (as value.right
is basically a closure to modifier
), then the conditions for finding reference in update_reference
will be failed as the node has been replaced for the next call.
update_reference(contexts, node, current_node, 'callee'); | ||
} else if (current_node.type === 'MemberExpression') { | ||
update_reference(contexts, node, current_node, 'object'); | ||
update_reference(contexts, node, current_node, 'property'); |
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.
I dont think you would want to update reference for MemberExpression#property
,
you might be updating the reference of foo
in a.foo
below.
{#each arrays as { foo, bar = a.foo } }
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.
Oh yeah, you're right, missed that one...
|
||
if (!current_node || !contexts.length) return; | ||
|
||
if (current_node.type === 'Identifier') { |
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.
probably can use estree-walker
to traverse down the node.
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.
you can use is-reference
to determine whether the Identifier is a reference or not.
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.
will try to use estree-walker
, I've seen it to be used in some codes there but hesitated since doing recursive looks more compelling...
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.
done
66debf4
to
56a03bd
Compare
56a03bd
to
0beee4e
Compare
Thanks for the PR, and sorry it took so long to get in! I'm closing this as these changes were incorporated into #5986, which has now been merged and released in 3.33.0. |
Fix #5066
Before submitting the PR, please make sure you do the following
npm run lint
!)This REPL as mentioned in {#each} block destructuring can't have default values refer to destructured params #5066 failed without this PR.
Tests
npm test
oryarn test
)