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 object rest in array pattern #10275

Merged

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Jul 27, 2019

Q                       A
Fixed Issues? Fixes #7215, Fixes #10257
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Edit by @JLHwung : Replace Fixes #7215, #10257 by Fixes #7215, Fixes #10257.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 28, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11525/

@JLHwung
Copy link
Contributor

JLHwung commented Jul 29, 2019

@tanhauhau Could you rebase on the master branch? The linting errors have been fixed.

@tanhauhau tanhauhau force-pushed the tanhauhau/object-rest-spread-array branch from 4fad08d to 13e5d69 Compare July 30, 2019 00:19
@JLHwung JLHwung added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jul 31, 2019
// for ({a, ...b} of []) {}
if (t.isObjectPattern(left) && hasRestElement(leftPath)) {
const temp = scope.generateUidIdentifier("ref");
if (hasObjectPatternRestElement(leftPath)) {
Copy link
Contributor

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 🤦‍♂️.

@JLHwung
Copy link
Contributor

JLHwung commented Jul 31, 2019

Could you change Fixed Issues to

Fixes #7215, Fixes #10257

I don't think GitHub supports Fixes comma separated issues.

@hzoo
Copy link
Member

hzoo commented Jul 31, 2019

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!

@tanhauhau tanhauhau force-pushed the tanhauhau/object-rest-spread-array branch from 13e5d69 to 088d220 Compare September 6, 2019 01:09
visitRestElements(path, restElement => {
if (restElement.parentPath.isObjectPattern()) {
foundRestElement = true;
path.stop();
Copy link
Member

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()?

Copy link
Member Author

@tanhauhau tanhauhau Sep 7, 2019

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?

Copy link
Member

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.

Copy link
Member Author

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

function hasRestElement(path) {
let foundRestElement = false;
visitRestElements(path, () => {
foundRestElement = true;
path.stop();
});
return foundRestElement;
}

does this means that this shouldnt use path.stop() either?

Copy link
Member

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

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: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Object Rest/Spread
Projects
None yet
5 participants