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

Spread operator does not implement iterator #6930

Closed
ksashikumar opened this Issue Nov 29, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@ksashikumar
Contributor

ksashikumar commented Nov 29, 2017

Choose one: is this a bug report or feature request?

Bug

Input Code

Array.prototype[Symbol.iterator] = function() {
  throw new Error();
}
var parts = ['shoulders', 'knees']; 
var lyrics = ['head', ...parts, 'and', 'toes']; 

Babel/Babylon Configuration (.babelrc, package.json, cli command)

presets -latest

Expected Behavior

➜ cat index.js
Array.prototype[Symbol.iterator] = function() {
  throw new Error();
}
var parts = ['shoulders', 'knees']; 
var lyrics = ['head', ...parts, 'and', 'toes'];                                                                                                                              

➜ d8 index.js
index.js:2: Error
  throw new Error();
  ^
Error
    at Array.(anonymous function) (index.js:2:9)
    at index.js:5:26

Current Behavior

No Error

Possible Solution

SpreadElement specification states, the AssignmentExpression's iterator is used to iterate. This should be transpiled to something similar to for...of ?
plugin-transform-spread should be rewritten?

Context

Your Environment

software version(s)
Babel
Babylon
node
npm
Operating System
@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Nov 29, 2017

Collaborator

Hey @ksashikumar! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Nov 29, 2017

Hey @ksashikumar! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Nov 29, 2017

Member

We assume you aren't going to modify built-ins, especially for an array like that. If you use our repl you'll find it compiles to a concat. Also babel-preset-latest is deprecated so I'd recommend using preset-env instead.

http://babeljs.io/docs/usage/caveats/ I'd say that is another caveat, we assume you haven't modified it in that way?

Or maybe this is because of the way we handle it with strings..

https://babeljs.io/repl/build/master#?babili=false&browsers=last%202%20Safari%20versions&build=&builtIns=true&code_lz=IIJxEME8DoAcQPYBdmVgUwNoGVIFsAjBAG2gEsl0IUQBdAAgF56AzAVwDsBjJMhDgBQBKegG8AUPXpIAFogDu9DukUBRMAhDCA3OIC-4gG7gQ9WCaQBnJvUwBySzIRtiAEyqW7AGnp2A1sronrTa9EYm9MSQIGRc1sz2Mujgrt700BnmIFY-duAcqbkoQXYhUuGm4DaYGdBRMXE-tVlWIUA&debug=true&circleciRepo=&evaluate=true&lineWrap=true&presets=es2015&prettier=false&targets=&version=7.0.0-beta.32

if you only use ... it uses our toConsumableArray helper which does an Array.from

Member

hzoo commented Nov 29, 2017

We assume you aren't going to modify built-ins, especially for an array like that. If you use our repl you'll find it compiles to a concat. Also babel-preset-latest is deprecated so I'd recommend using preset-env instead.

http://babeljs.io/docs/usage/caveats/ I'd say that is another caveat, we assume you haven't modified it in that way?

Or maybe this is because of the way we handle it with strings..

https://babeljs.io/repl/build/master#?babili=false&browsers=last%202%20Safari%20versions&build=&builtIns=true&code_lz=IIJxEME8DoAcQPYBdmVgUwNoGVIFsAjBAG2gEsl0IUQBdAAgF56AzAVwDsBjJMhDgBQBKegG8AUPXpIAFogDu9DukUBRMAhDCA3OIC-4gG7gQ9WCaQBnJvUwBySzIRtiAEyqW7AGnp2A1sronrTa9EYm9MSQIGRc1sz2Mujgrt700BnmIFY-duAcqbkoQXYhUuGm4DaYGdBRMXE-tVlWIUA&debug=true&circleciRepo=&evaluate=true&lineWrap=true&presets=es2015&prettier=false&targets=&version=7.0.0-beta.32

if you only use ... it uses our toConsumableArray helper which does an Array.from

@ksashikumar

This comment has been minimized.

Show comment
Hide comment
@ksashikumar

ksashikumar Nov 30, 2017

Contributor

@hzoo Actually, I was trying to find the root cause of this bug: #5147. I was playing around with spread operator and found this issue. babel-plugin-transform-spread just transpiles the spread in array to concat. My question is, if v8's internal implementation of SpreadElement uses the iterator of the expression, should babel also not transpile using iterator?

Contributor

ksashikumar commented Nov 30, 2017

@hzoo Actually, I was trying to find the root cause of this bug: #5147. I was playing around with spread operator and found this issue. babel-plugin-transform-spread just transpiles the spread in array to concat. My question is, if v8's internal implementation of SpreadElement uses the iterator of the expression, should babel also not transpile using iterator?

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Dec 2, 2017

Contributor

I wonder if Babel should always use Array.from in these situations? That will then use the iterator without the need for writing any customs code on Babel's part. Of course, the problem is that this will be significantly worse for performance than the current concat or for loop code.

Contributor

apapirovski commented Dec 2, 2017

I wonder if Babel should always use Array.from in these situations? That will then use the iterator without the need for writing any customs code on Babel's part. Of course, the problem is that this will be significantly worse for performance than the current concat or for loop code.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Dec 2, 2017

Member

Array.from is a part of es6 and forcing people to use global polyfills to use spread operator doesn't seem to be right.

Member

Andarist commented Dec 2, 2017

Array.from is a part of es6 and forcing people to use global polyfills to use spread operator doesn't seem to be right.

@apapirovski

This comment has been minimized.

Show comment
Hide comment
@apapirovski

apapirovski Dec 2, 2017

Contributor

@Andarist I mean, it's already using Array.from in the helper, it just checks whether it's an Array first and branches based on that. Just musing here...

Contributor

apapirovski commented Dec 2, 2017

@Andarist I mean, it's already using Array.from in the helper, it just checks whether it's an Array first and branches based on that. Just musing here...

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Dec 2, 2017

Member

Oh, I always forget about normal mode :D using loose exclusively in my projects.

Member

Andarist commented Dec 2, 2017

Oh, I always forget about normal mode :D using loose exclusively in my projects.

@ksashikumar ksashikumar closed this Mar 3, 2018

@lock lock bot added the outdated label Jun 2, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2018

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