Skip to content

Fix: destructuring assignment with an empty array in object #5000

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.

Sign up for GitHub

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

Jump to bottom
Merged
GeoffreyBooth merged 3 commits into from
Mar 10, 2018
Merged

Fix: destructuring assignment with an empty array in object #5000

GeoffreyBooth merged 3 commits into from
Mar 10, 2018

Conversation

@zdenko
Copy link
Collaborator

@zdenko zdenko commented Mar 3, 2018 •
edited
Loading

While testing sub-structuring (#4995) I noticed wrong output of destructuring with an empty array in the object.

Current branch:

{a:[], b} = obj

###
var b;
obj.undefined, b = obj[1];
###

Since @variable.isAssignable() in Assign returns false because of an empty array, the ::compileDestructuring is invoked.
The syntax is valid and can be output directly.

This PR:

{a:[], b} = obj

###
var b;
({
  a: [],
  b
} = c);
###

@zdenko zdenko force-pushed the obj_arr_destruct branch from cbd5fe2 to 2306d62 Compare March 3, 2018 20:40
@aminland
Copy link

aminland commented Mar 7, 2018

This would be a breaking change since babel runs {a: []} through function _toArray(arr) { return Array.isArray(arr) ? arr : Array.from(arr); }.

As an example Array.from(null) and Array.from(undefined) throws a type error, so anyone who was using it to drop keys will be screwed.

@zdenko
Copy link
Collaborator Author

zdenko commented Mar 9, 2018

I think my comment from #4995 applies here as well.
I see no difference between {a: []} = b or {a:{}} = b. It is upon user to take care of destructuring variables.
And, I don't think anyone will be screwed, because currently, if an empty array is used in the destructuring`, you won't get correct result even if the key exists in the object.

@GeoffreyBooth
Copy link
Collaborator

GeoffreyBooth commented Mar 9, 2018

I don’t think we should care what Babel does. If we output valid JS, that’s all we need to worry about.

This is correct, isn’t it?

obj = {a: [1]}
result = ({a: []} = obj)

In Chrome console, result equals {a: [1]}.

So @zdenko and @aminland is this safe to merge in?

@zdenko
Copy link
Collaborator Author

zdenko commented Mar 10, 2018

@GeoffreyBooth go for it.

@GeoffreyBooth GeoffreyBooth merged commit 1869f31 into jashkenas:master Mar 10, 2018
@GeoffreyBooth GeoffreyBooth mentioned this pull request Mar 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zdenko @aminland @GeoffreyBooth