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

[CS2] destructured nested defaults [fixes #4566] #4574

Merged

Conversation

helixbass
Copy link
Collaborator

Fixes #4566

Added recursive assignment of @lhs to destructured objects (corresponding to the recursive assignment of @lhs that was already there for arrays)

Looks like this won't cause a merge conflict with #4493

@GeoffreyBooth
Copy link
Collaborator

This is very elegant. Looks good to me. @lydell or @connec, do you care to review?

@helixbass
Copy link
Collaborator Author

@GeoffreyBooth I actually just stumbled on another similar nested destructuring bug eg {a: {b} = {}} = c was generating:

({
  a: ({b} = {})
} = c);

So I added another commit to suppress the Assign-wrapping parentheses when it's nested like that

@@ -1810,7 +1821,7 @@ exports.Assign = class Assign extends Base
answer = compiledName.concat @makeCode(" #{ @context or '=' } "), val
# Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration,
# if we’re destructuring without declaring, the destructuring assignment must be wrapped in parentheses.
if o.level > LEVEL_LIST or (isValue and @variable.base instanceof Obj and not @param)
if o.level > LEVEL_LIST or (isValue and @variable.base instanceof Obj and not @nestedLhs and not @param)
Copy link
Collaborator

@connec connec Jun 14, 2017

Choose a reason for hiding this comment

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

I take it @variable.base.lhs instead if @nestedLhs is not adequate (and/or desirable) here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@connec nope it seems to interfere w/ the wrapping in parens of top-level destructured object assigns. Not crystal clear on the different nested Obj/Assign structures so there may be a way do it without introducing nestedLhs but that's the way I found to distinguish the top-level Assigns from the nested ones

@GeoffreyBooth GeoffreyBooth merged commit 26f6fa6 into jashkenas:2 Jun 21, 2017
@GeoffreyBooth GeoffreyBooth modified the milestone: 2.0.0 Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants