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

Don't extract rest elements from nested expressions #7364

Merged
merged 2 commits into from
Feb 17, 2018

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 10, 2018

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

The problem was that the plugin didn't only traverse the destructuring pattern when searching for rest elements, but also default values.

I suggest to review this PR ignoring whitespaces (https:/babel/babel/pull/7364/files?w=1)

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 10, 2018

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

});
return foundRestElement;
}

function visitRestElements(path, visitor) {
Copy link
Member

Choose a reason for hiding this comment

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

I realize that this is probably more performant solution, but the code itself is imho a little bit convoluted, hard to read and to reason about. It has a lot of indirection in it. Would it be possible to rephrase it into a simpler implementation?

const stop = visitor(path);
if (stop) return;
path.traverse({
Expression(path) {
Copy link
Member

Choose a reason for hiding this comment

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

this is way easier to track! ❤️

@Andarist
Copy link
Member

@nicolo-ribaudo I think this caused the merge conflict now for you - #7388

@nicolo-ribaudo nicolo-ribaudo changed the title Don't extract rest elements from nested functions Don't extract rest elements from nested expressions Feb 17, 2018
@existentialism existentialism added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Object Rest/Spread labels Feb 17, 2018
@Andarist Andarist merged commit 3d49766 into babel:master Feb 17, 2018
@nicolo-ribaudo nicolo-ribaudo deleted the obj-rest-nested-function branch February 17, 2018 17:41
aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018
* Don't extract rest elements from nested expressions

* Node 4
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
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
Development

Successfully merging this pull request may close these issues.

Duplicate declaration error for exported functions with object destructuring arguments
4 participants