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 object rest in array pattern #10275
fix object rest in array pattern #10275
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11525/ |
3ebe839
to
4fad08d
Compare
@tanhauhau Could you rebase on the master branch? The linting errors have been fixed. |
4fad08d
to
13e5d69
Compare
// for ({a, ...b} of []) {} | ||
if (t.isObjectPattern(left) && hasRestElement(leftPath)) { | ||
const temp = scope.generateUidIdentifier("ref"); | ||
if (hasObjectPatternRestElement(leftPath)) { |
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.
nit: prefer early return here for less nested blocks, and more effective diffs 🤦♂️.
Could you change Fixed Issues to
I don't think GitHub supports |
Yeah @JLHwung is correct, it's required to put a word after for each number https://help.github.com/en/articles/closing-issues-using-keywords for ref! |
13e5d69
to
088d220
Compare
visitRestElements(path, restElement => { | ||
if (restElement.parentPath.isObjectPattern()) { | ||
foundRestElement = true; | ||
path.stop(); |
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.
Shouldn't this be restElement.stop()
?
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.
if i understand correctly, restElement.stop()
will stop the traversing into restElement
but continue on siblings, but path.stop()
will stop the entire visitRestElements
traversal?
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.
That should be the difference between .skip()
and .stop()
. path
is not generated by visitElemenrs
: you should use it when you want to stop the traversal which provided 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.
oh. understood.
I found out that this line was copied over from
babel/packages/babel-plugin-proposal-object-rest-spread/src/index.js
Lines 31 to 38 in 088d220
function hasRestElement(path) { | |
let foundRestElement = false; | |
visitRestElements(path, () => { | |
foundRestElement = true; | |
path.stop(); | |
}); | |
return foundRestElement; | |
} |
does this means that this shouldnt use path.stop()
either?
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.
Yeah, that looks like an error
Edit by @JLHwung : Replace
Fixes #7215, #10257
byFixes #7215, Fixes #10257
.