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

Type checking support for Array Iterator #3553

Closed
danahartweg opened this Issue Feb 27, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@danahartweg

danahartweg commented Feb 27, 2017

Description

When using Babel, specifically with array polyfills for iterator support, all array prototypes are modified for that support. Therefore, toString.call(obj) reports as [object Array Iterator] instead of simply [object Array], which jQuery has no awareness of in the class2type map. The goal would be to update the class2type map for that support so these arrays report themselves as arrays instead of objects.

I know there has been some contention with this type checking in the past, specifically around the Error object, so I decided to open an issue for discussion before submitting a PR with the changes proposed below.

Proposed changes

Not sure the best way to provide a test case for this. Instead, this is the code change I had in mind.

@mgol

This comment has been minimized.

Show comment
Hide comment
@danahartweg

This comment has been minimized.

Show comment
Hide comment
@danahartweg

danahartweg Feb 28, 2017

I'll definitely work on getting an example together / tracking down the exact part of Babel that is causing the problem. Things have been crazy since I submitted this issue originally.

danahartweg commented Feb 28, 2017

I'll definitely work on getting an example together / tracking down the exact part of Babel that is causing the problem. Things have been crazy since I submitted this issue originally.

@mgol mgol added the Needs info label Mar 1, 2017

@danahartweg

This comment has been minimized.

Show comment
Hide comment
@danahartweg

danahartweg Mar 5, 2017

Very sorry for the additional delay, but I've finally been able to do some digging. It appears that this is an issue specifically when using the babel-transform-runtime plugin instead of babel-polyfill.

danahartweg commented Mar 5, 2017

Very sorry for the additional delay, but I've finally been able to do some digging. It appears that this is an issue specifically when using the babel-transform-runtime plugin instead of babel-polyfill.

@markelog markelog added Needs review Core and removed Needs info labels Mar 5, 2017

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 27, 2017

Member

@danahartweg could you provide a simple example project where it fails?

Member

mgol commented Mar 27, 2017

@danahartweg could you provide a simple example project where it fails?

@danahartweg

This comment has been minimized.

Show comment
Hide comment
@danahartweg

danahartweg Apr 2, 2017

@mgol I was unable to reproduce the failure in a clean project with the exact same dependencies from the project I initially noticed the issue. Given the size of the main project, there is obviously some other condition causing babel-transform-runtime to pollute the Array prototypes.

Sorry for taking up your time with this. I'm going to close the issue until I can (if ever) nail down the exact condition babel is hitting.

danahartweg commented Apr 2, 2017

@mgol I was unable to reproduce the failure in a clean project with the exact same dependencies from the project I initially noticed the issue. Given the size of the main project, there is obviously some other condition causing babel-transform-runtime to pollute the Array prototypes.

Sorry for taking up your time with this. I'm going to close the issue until I can (if ever) nail down the exact condition babel is hitting.

@danahartweg danahartweg closed this Apr 2, 2017

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 2, 2017

Member

Thanks for an update! Let us know if you ever manage to reproduce it in a minimal project.

Member

mgol commented Apr 2, 2017

Thanks for an update! Let us know if you ever manage to reproduce it in a minimal project.

@bvarga

This comment has been minimized.

Show comment
Hide comment
@bvarga

bvarga Apr 11, 2017

Just a note.

Here you can find a detailed description of the problem.
Mainly you need an old 1.7.0 prototype.js to break things. however It's fixed in 1.7.3.

regards
Balazs

bvarga commented Apr 11, 2017

Just a note.

Here you can find a detailed description of the problem.
Mainly you need an old 1.7.0 prototype.js to break things. however It's fixed in 1.7.3.

regards
Balazs

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

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