Skip to content

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

Merged
GeoffreyBooth merged 3 commits into
jashkenas:masterfrom
zdenko:obj_arr_destruct
Mar 10, 2018
Merged

Fix: destructuring assignment with an empty array in object#5000
GeoffreyBooth merged 3 commits into
jashkenas:masterfrom
zdenko:obj_arr_destruct

Conversation

@zdenko

@zdenko zdenko commented Mar 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

aminland commented Mar 7, 2018

Copy link
Copy Markdown

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

zdenko commented Mar 9, 2018

Copy link
Copy Markdown
Collaborator Author

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

GeoffreyBooth commented Mar 9, 2018

Copy link
Copy Markdown
Collaborator

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

zdenko commented Mar 10, 2018

Copy link
Copy Markdown
Collaborator Author

@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