Skip to content

transform-spread: fix behaviour if array is hole#8872

Open
macabeus wants to merge 1 commit into
babel:masterfrom
macabeus:fix/babel-plugin-transform-spread
Open

transform-spread: fix behaviour if array is hole#8872
macabeus wants to merge 1 commit into
babel:masterfrom
macabeus:fix/babel-plugin-transform-spread

Conversation

@macabeus

@macabeus macabeus commented Oct 14, 2018

Copy link
Copy Markdown
Contributor
Q                       A
Fixed Issues? Fixes #7258
Patch: Bug Fix? Yeah
Major: Breaking Change? Yep (if someone uses the incorrect behaviour)
Minor: New Feature?
Tests Added + Pass? Yep
Documentation PR Link
Any Dependency Changes?
License MIT

Babel wasn't compile correctly if array is hole. For example:

Correct behaviour:

var array = new Array(3);
[...array].forEach(console.log); // should print undefined three times

var array2 = [1,,,77,,, 99];
[...array2]; // should be [1, undefined, undefined, 77, undefined, undefined, 99]

Babel wrong behaviour fixed:

var array = new Array(3);
[...array].forEach(console.log); // print undefined only once!!

var array2 = [1,,,77,,, 99];
[...array2] // is [1, 3: 77, 6: 99] !!

@macabeus macabeus changed the title transform-spread: fix empty array behaviour transform-spread: fix hole array behaviour Oct 14, 2018
@macabeus macabeus force-pushed the fix/babel-plugin-transform-spread branch from b4020fe to 1c122c9 Compare October 14, 2018 04:06
@macabeus macabeus changed the title transform-spread: fix hole array behaviour transform-spread: fix behaviour if array is hole Oct 14, 2018
@babel-bot

babel-bot commented Oct 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

@babel-bot

babel-bot commented Oct 14, 2018

Copy link
Copy Markdown
Collaborator

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

loganfsmyth
loganfsmyth reviewed Oct 14, 2018
View reviewed changes
Comment thread packages/babel-plugin-transform-spread/src/index.js Outdated
this.addHelper("toConsumableArray");

const toConsumableArrayExpression = obj =>
t.callExpression(t.identifier("_toConsumableArray"), obj);

@loganfsmyth loganfsmyth Oct 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't have time to think about this PR as a whole at the moment, but FYI this identifier is returned from addHelper, you shouldn't create it manually.

@macabeus macabeus force-pushed the fix/babel-plugin-transform-spread branch from 1c122c9 to 7c480aa Compare October 14, 2018 17:56
@macabeus macabeus force-pushed the fix/babel-plugin-transform-spread branch from 7c480aa to 05c6350 Compare October 14, 2018 17:59
@macabeus

macabeus commented Oct 14, 2018

Copy link
Copy Markdown
Contributor Author

@loganfsmyth Nice! I updated the code to use the return of addHelper, also I made some readable improvements and fix some others edge case

@macabeus macabeus mentioned this pull request Oct 14, 2018
Closed
@nicolo-ribaudo nicolo-ribaudo added the PR: Spec Compliance 👓 A type of pull request used for our changelog categories label Oct 19, 2018
nicolo-ribaudo
nicolo-ribaudo reviewed Oct 19, 2018
View reviewed changes
Comment thread packages/babel-plugin-transform-spread/src/index.js
}

path.replaceWith(
const toConsumableArray = this.addHelper("toConsumableArray");

@nicolo-ribaudo nicolo-ribaudo Oct 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should move this call inside the wrapCallConsumableArray function, otherwise we are going to reuse the same node multiple times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loganfsmyth loganfsmyth loganfsmyth left review comments

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo left review comments

At least 6 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

PR: Spec Compliance 👓 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect compilation for empty array

4 participants

@macabeus @babel-bot @loganfsmyth @nicolo-ribaudo