Skip to content

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