Add iterator support #27

JakeChampion · 2016-06-22T21:08:03Z

Should solve #4

ljharb · 2016-06-23T00:14:39Z

implementation.js

+		while (!done) {
+			iterableResponse = arrayLike.next();
+			if (
+				iterableResponse.hasOwnProperty('value')


let's use the "has" module rather than trusting Object#hasOwnProperty

JakeChampion · 2016-07-15T15:05:13Z

@ljharb Addressed all comments raised.

ljharb · 2016-07-16T05:00:07Z

implementation.js

@@ -1,9 +1,88 @@
 'use strict';
 var ES = require('es-abstract/es6');
 var supportsDescriptors = require('define-properties').supportsDescriptors;
+var has = require('has');
+var global = require('global-object');


an alternative to this module would be https://www.npmjs.com/package/system.global which follows the official language spec proposal.

+1 to using system.global instead.

mathiasbynens · 2016-08-11T13:22:59Z

What’s with the failing builds?

ljharb · 2016-08-11T16:24:15Z

Most of the new tests in this PR are missing explicit end or plan calls.

JakeChampion · 2016-08-11T20:17:19Z

All passing now :-)

JakeChampion · 2016-08-14T01:19:55Z

@ljharb This is ready for another review when you have the time. Thanks for helping with this feature by the way, I really appreciate the feedback you have given :-)

ljharb · 2016-08-14T02:10:36Z

implementation.js

+	}());
+
+	if (supportsStrIterator) {
+		iteratorSymbol = '@@iterator';


edit: now fixed ~~I don't think that the above check actually indicates that it supports a string iterator - that can only be tested by setting one, and asserting that for..of invokes it.~~

ljharb · 2016-08-14T03:12:37Z

implementation.js

+	if (iteratorSymbol && isCallable(items[iteratorSymbol])) {
+		return items[iteratorSymbol]();
+	} else if ('Set' in global && 'Map' in global && isCallable(items.entries) && isCallable(items.values)) {
+		if (items.constructor === Set) {


anything with === won't work cross-realm (like iframes, or web workers, etc) - to be robust, I think you'd need to stash a copy of the Set.prototype.size getter (and Map) and assert that .calling on items did not throw.

I made a JSBin to test this out, it seems Sets and Maps are detectable across realms (I tested web workers) via instanceof. Is this what you expected?

http://jsbin.com/kiguqunoxi/edit?js,console

Another, the following test will work on many more browsers.

var iframe = document.createElement('iframe'); iframe.src = 'about:blank'; document.body.appendChild(iframe); console.log(isMap(new iframe.contentWindow.Map()));

If that's the case with instanceof, it's a violation of the spec - the .call you're using is the correct way to make it work across realms.

Yes, it is assumed to use .call.

JakeChampion · 2016-08-16T22:58:59Z

Addressed all comments, all passing on CI except for 3 allowed failures -- node 0.9, node 0.6, node 0.4

I'm currently running this test suite against a multitude of browsers, this usually takes a very long time. I will add a comment Wednesday morning (UK time). So far, it looks like there are errors on ios 8.2 :-/

JakeChampion · 2016-08-16T23:41:09Z

implementation.js

+	if (has(items, iteratorSymbol)) {
+		return items[iteratorSymbol]();
+	} else if ('Set' in global && 'Map' in global && isCallable(items.entries) && isCallable(items.values)) {
+		if (items.constructor === Set) {


Won't work cross realm, need to resolve this.

JakeChampion · 2016-08-17T07:55:32Z

All browsers not finished running yet, results so far for browsers which failed:

- failed: <iphone 7.0 on Mac 10.9> (4 failed, 96 passed)
- failed: <iphone 7.1 on Mac 10.10> (4 failed, 96 passed)
- failed: <iphone 8.1 on Mac 10.10> (12 failed, 88 passed)
- failed: <iphone 8.2 on Mac 10.10> (12 failed, 88 passed)
- failed: <iphone 8.4 on Mac 10.10> (12 failed, 88 passed)
- failed: <iphone 9.1 on Mac 10.10> (4 failed, 96 passed)
- failed: <iphone 9.2 on Mac 10.10> (4 failed, 96 passed)
- failed: <iphone 9.0 on Mac 10.10> (4 failed, 96 passed)
- failed: <ipad 7.0 on Mac 10.9> (4 failed, 96 passed)
- failed: <ipad 7.1 on Mac 10.10> (4 failed, 96 passed)
- failed: <iphone 9.3 on Mac 10.11> (4 failed, 96 passed)
- failed: <ipad 8.2 on Mac 10.10> (12 failed, 88 passed)
- failed: <ipad 8.1 on Mac 10.10> (12 failed, 88 passed)
- failed: <ipad 8.3 on Mac 10.10> (12 failed, 88 passed)
- failed: <ipad 8.4 on Mac 10.10> (12 failed, 88 passed)
- failed: <ipad 9.0 on Mac 10.10> (4 failed, 96 passed)
- failed: <ipad 9.1 on Mac 10.10> (4 failed, 96 passed)
- failed: <ipad 9.2 on Mac 10.10> (4 failed, 96 passed)
- failed: <firefox 20 on Windows 2008> (4 failed, 96 passed)
- failed: <ipad 9.3 on Mac 10.11> (4 failed, 96 passed)
- failed: <firefox 22 on Windows 2012 R2> (4 failed, 96 passed)
- failed: <firefox 21 on Mac 10.9> (4 failed, 96 passed)
- failed: <firefox 25 on Windows 2008> (8 failed, 92 passed)
- failed: <firefox 24 on Windows 2012> (8 failed, 92 passed)
- failed: <firefox 26 on Windows 10> (8 failed, 92 passed)
- failed: <firefox 23 on Mac 10.11> (4 failed, 96 passed)
- failed: <safari 6 on Mac 10.8> (12 failed, 88 passed)
- failed: <safari 8 on Mac 10.10> (12 failed, 88 passed)
- failed: <safari 7 on Mac 10.9> (4 failed, 96 passed)
- failed: <safari 9 on Mac 10.11> (4 failed, 96 passed)
- failed: <opera 11 on Windows 2003> (4 failed, 96 passed)
- failed: <opera 12 on Windows 2003> (4 failed, 96 passed)

falsandtru · 2016-08-18T14:50:15Z

implementation.js

+	return done ? tempArray : false;
+};
+
+var hasSymbols = typeof Symbol === 'function' && typeof Symbol.iterator === 'symbol';


typeof Symbol.iterator === 'symbol' doesn't work with Symbol polyfills.

That's because Symbol isn't truly polyfillable - it's a native-only feature. We should be supporting shims, but not go out of our way to support shams.

falsandtru · 2016-08-18T22:02:21Z

I refined this support in #30. It would work better. Could you review it?

JakeChampion · 2016-08-18T23:46:04Z

I ran the tests against the master using my list of browsers, it seems to have the same 4 failing tests.

JakeChampion · 2016-08-18T23:46:50Z

   - failed: <iphone 7.0 on Mac 10.9> (4 failed, 80 passed)
    1 browser(s) failed
  Failed Tests: There were 4 failures
    no setters are called for indexes
      ✖ should throw
    no setters are called for indexes
      ✖ should throw
    no setters are called for indexes
      ✖ should throw
    no setters are called for indexes
      ✖ should throw
  total:     261
  passing:   257
  failing:   4
  duration:  1m 15.1s
~/Code/oss/Array.from master 1m 15s

JakeChampion · 2016-08-19T00:16:16Z

I've commented out the test which is failing on master and am now rerunning against all the browsers on SauceLabs. I have high hopes it will work :-)

ljharb · 2016-08-19T03:25:58Z

Hmm - I'd like to fix those tests on master before merging this.

JakeChampion · 2016-08-19T21:13:26Z

🎉 🎉 🎉 🎉 🎉 🎉 🎉

- passed: <iphone 7.0 on Mac 10.9>
- passed: <iphone 7.1 on Mac 10.10>
- passed: <iphone 9.0 on Mac 10.10>
- passed: <iphone 9.1 on Mac 10.10>
- passed: <iphone 9.2 on Mac 10.10>
- passed: <iphone 9.3 on Mac 10.11>
- passed: <ipad 7.0 on Mac 10.9>
- passed: <ipad 7.1 on Mac 10.10>
- passed: <ipad 9.0 on Mac 10.10>
- passed: <ipad 9.1 on Mac 10.10>
- passed: <ipad 9.2 on Mac 10.10>
- passed: <ipad 9.3 on Mac 10.11>
- passed: <firefox 4 on Windows 10>
- passed: <firefox 5 on Windows 2008>
- passed: <firefox 6 on Mac 10.8>
- passed: <firefox 7 on Windows 10>
- passed: <firefox 8 on Mac 10.11>
- passed: <firefox 9 on Windows 2012 R2>
- passed: <firefox 10 on Mac 10.8>
- passed: <firefox 11 on Windows 2008>
- passed: <firefox 12 on Windows 2012>
- passed: <firefox 45 on Windows 10>
- passed: <firefox 46 on Mac 10.11>
- passed: <firefox 47 on Mac 10.11>
- passed: <firefox 48 on Windows 2012>
- passed: <safari 5 on Windows 2008>
- passed: <safari 7 on Mac 10.9>
- passed: <safari 9 on Mac 10.11>
- passed: <chrome 26 on Windows 10>
- passed: <chrome 27 on Linux>
- passed: <chrome 28 on Windows 2003>
- passed: <chrome 29 on Windows 10>
- passed: <chrome 50 on Mac 10.9>
- passed: <chrome 51 on Mac 10.10>
- passed: <chrome 52 on Mac 10.10>
- passed: <internet explorer 9 on Windows 2008>
- passed: <internet explorer 10 on Windows 2012>
- passed: <microsoftedge 13 on Windows 10>
- passed: <opera 11 on Windows 2003>
- passed: <opera 12 on Windows 2003>
- passed: <android 4.0 on Linux>
- passed: <android 4.1 on Linux>
- passed: <android 4.3 on Linux>
- passed: <android 4.2 on Linux>
- passed: <android 4.4 on Linux>
- passed: <android 5.0 on Linux>
- passed: <android 5.1 on Linux>

JakeChampion · 2016-09-14T23:32:51Z

@mathiasbynens @ljharb I believe this is ready for it's final review :-)

ljharb · 2016-09-15T02:06:19Z

@mathiasbynens btw would you mind enabling required PR approvals on this repo, in settings? :-)

@JakeChampion thanks! I'll take a final look and merge tonight.

ljharb · 2016-09-15T05:55:04Z

Thanks! I'm going to give this a couple days to bake, and for me to test and play around, before releasing it.

le0nik · 2018-02-06T18:35:48Z

Iterators still don't work in latest as it is 1.0.3. Should users just install master until this is released?

mroderick · 2018-07-22T13:20:19Z

Or use https://github.com/studio-b12/array-from until a new version is published to NPM

JakeChampion mentioned this pull request Jun 22, 2016

Doesn't support FileList #26

Closed

ljharb reviewed Jun 23, 2016
View reviewed changes

ljharb reviewed Jul 16, 2016
View reviewed changes

ljharb reviewed Aug 14, 2016
View reviewed changes

JakeChampion reviewed Aug 16, 2016
View reviewed changes

JakeChampion mentioned this pull request Aug 18, 2016

Fix Array.from static method polyfillpolyfill/polyfill-service#840

Closed

falsandtru reviewed Aug 18, 2016
View reviewed changes

ljharb mentioned this pull request Aug 18, 2016

Add iterable support #30

Closed

falsandtru mentioned this pull request Aug 22, 2016

Incorrectness of Array.from with iterators polyfillpolyfill/polyfill-service#844

Closed

Add support for iterators

afb0626

ljharb added the enhancement label Sep 15, 2016

ljharb merged commit 0322091 into mathiasbynens:master Sep 15, 2016

ljharb mentioned this pull request Sep 15, 2016

Support iterators #4

Closed

JakeChampion deleted the iteration branch September 15, 2016 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iterator support #27

Add iterator support #27

JakeChampion commented Jun 22, 2016 •

edited

ljharb Jun 23, 2016

JakeChampion commented Jul 15, 2016

ljharb Jul 16, 2016

mathiasbynens Jul 16, 2016

mathiasbynens commented Aug 11, 2016

ljharb commented Aug 11, 2016

JakeChampion commented Aug 11, 2016

JakeChampion commented Aug 14, 2016

ljharb Aug 14, 2016 •

edited

ljharb Aug 14, 2016

JakeChampion Sep 13, 2016

falsandtru Sep 13, 2016

ljharb Sep 13, 2016

falsandtru Sep 13, 2016

JakeChampion commented Aug 16, 2016

JakeChampion Aug 16, 2016

JakeChampion commented Aug 17, 2016

falsandtru Aug 18, 2016

ljharb Aug 18, 2016

falsandtru commented Aug 18, 2016

JakeChampion commented Aug 18, 2016

JakeChampion commented Aug 18, 2016

JakeChampion commented Aug 19, 2016

ljharb commented Aug 19, 2016

JakeChampion commented Aug 19, 2016

JakeChampion commented Sep 14, 2016

ljharb commented Sep 15, 2016

ljharb commented Sep 15, 2016

le0nik commented Feb 6, 2018

mroderick commented Jul 22, 2018

Add iterator support #27

Add iterator support #27

Conversation

JakeChampion commented Jun 22, 2016 • edited

Choose a reason for hiding this comment

JakeChampion commented Jul 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiasbynens commented Aug 11, 2016

ljharb commented Aug 11, 2016

JakeChampion commented Aug 11, 2016

JakeChampion commented Aug 14, 2016

ljharb Aug 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeChampion commented Aug 16, 2016

Choose a reason for hiding this comment

JakeChampion commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falsandtru commented Aug 18, 2016

JakeChampion commented Aug 18, 2016

JakeChampion commented Aug 18, 2016

JakeChampion commented Aug 19, 2016

ljharb commented Aug 19, 2016

JakeChampion commented Aug 19, 2016

JakeChampion commented Sep 14, 2016

ljharb commented Sep 15, 2016

ljharb commented Sep 15, 2016

le0nik commented Feb 6, 2018

mroderick commented Jul 22, 2018

JakeChampion commented Jun 22, 2016 •

edited

ljharb Aug 14, 2016 •

edited