Skip to content
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

[BUGFIX beta] Align Em.isArray behavior with Array.isArray for FileList type #12708

Merged
merged 1 commit into from
Nov 10, 2016
Merged

[BUGFIX beta] Align Em.isArray behavior with Array.isArray for FileList type #12708

merged 1 commit into from
Nov 10, 2016

Conversation

kamilogorek
Copy link
Contributor

FileList type is currently detected as array, however it doesn't provide it's methods like forEach or map.
Also Array.isArray doesn't consider FileType as an array.

This PR unifies this behavior and fixes #12688.

@kamilogorek
Copy link
Contributor Author

It appears that IE9 test is failing because of FileList API absence.

@stefanpenner
Copy link
Member

If you are looking for an array, is there a reason to not use Array.isArray? What scenario lead you to discover this quirk?

We need to be careful that his doesn't have unexpected side affects, are there scenarios were the current check results in behavior someone legitimately relies on? What other cases are missing. Is this form of type checking actually flawed?

@kamilogorek
Copy link
Contributor Author

@stefanpenner CCing @joelcox from this issue, as he is the one who found this culprit #12688

@joelcox
Copy link
Contributor

joelcox commented Dec 20, 2015

Thanks for working on this fix @kamilogorek. I encountered this behavior while working on an internal file upload component (based on ember-uploader), which was set up in such a way that it would accept both an array of files, or a single file.

@homu
Copy link
Contributor

homu commented Jun 15, 2016

☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts.

@mixonic
Copy link
Member

mixonic commented Jun 26, 2016

@kamilogorek can you just move the fileList assertions into a separate test that only runs if fileList support is present? That would seem sufficient.

@kamilogorek
Copy link
Contributor Author

kamilogorek commented Jun 26, 2016

Sure, will do! I'll update PR within 24h when I get home.
Ok, maybe a little longer as I don't have an access to my laptop before Monday, sorry...

@kamilogorek
Copy link
Contributor Author

@mixonic like this?

@mixonic
Copy link
Member

mixonic commented Jul 5, 2016

Yeah this looks great to me @kamilogorek. r+ @rwjblue?

@homu
Copy link
Contributor

homu commented Sep 5, 2016

☔ The latest upstream changes (presumably #14215) made this pull request unmergeable. Please resolve the merge conflicts.

@kamilogorek
Copy link
Contributor Author

kamilogorek commented Sep 6, 2016

Resolved.

@rwjblue rwjblue merged commit 2999526 into emberjs:master Nov 10, 2016
@rwjblue
Copy link
Member

rwjblue commented Nov 10, 2016

Thanks @kamilogorek!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ember.isArray return true for FileList object
8 participants