Implements the JavaScript Iterable protocol. #4061

leebyron · 2016-07-20T22:50:44Z

This introduces new methods related to Iterators on Backbone.Collection to mirror those found on Array: values, keys, entries, and @@iterator. Each of these methods will return a JavaScript Iterator, which has a next method, yielding the models or ids of models contained in the Collection.

The CollectionIterator is careful to use the at() and modelId() methods on the host collection rather than direct access to the models property, which should ensure it is resilient to creative subclassing of Backbone.Collection and future feature addition.

CollectionIterator's next method follows the same algorithm defined by ES6 for ArrayIterator's next.

The @@iterator method is defined using Symbol.iterator if it exists in the JavaScript runtime (modern browsers/node.js) and falls back to the string "@@iterator" which was popularized by older versions of Firefox and has become the standard fallback behavior for other third-party libraries. This ensures that Backbone can still be used across all browsers, even with use of these new methods.

Supporting Iterable allows better integration between Backbone and the most recent additions to the JavaScript language, including for of loops and data-collection constructor functions, as well as better integration with other third-party libraries that accept Iterables instead of only Arrays.

Fixes #3954

jdalton · 2016-07-21T01:15:42Z

What I don't like about this is the fallback use of '@@iterator' since it introduces an inconsistency in older and newer enviros in that older enviros will have an enumerable string key, '@@iterator', while newer enviros will have a symbol key (which isn't included in things like Object.keys).

I know this has caused some bugs in Ramda (a lib implementing this style too).

I'm fine with it being a modern only enhancement (that's what lodash does).

leebyron · 2016-07-21T01:26:18Z

That's definitely a concern to weigh. I'm curious to know more about the issues Ramda encountered. I would be surprised if Object.keys() presented an issue, since that's a pretty uncommon operation in general on a Backbone.Collection but even if it was performed, it should not include the properties of the Collection.prototype (unless of course you're calling Object.keys() directly on Collection.prototype). If this is a serious concern, we could use Object.defineProperty instead to ensure it is non-enumerable.

Another potential approach to avoid inconsistency is to always define Collection.prototype["@@iterator"] while only defining Collection.prototype[Symbol.iterator] should Symbol.iterator be defined.

An unfortunate issue of only implementing Iterable when Symbol is defined is the potential for inconsistent behavior between browsers when in use with libraries that support the fallback (Ramda being a fine example).

jdalton · 2016-07-21T02:14:27Z

I'm curious to know more about the issues Ramda encountered.

The issue here or here.

An unfortunate issue of only implementing Iterable when Symbol is defined is the potential for inconsistent behavior between browsers when in use with libraries that support the fallback (Ramda being a fine example).

Naw, there wouldn't be an issue because Backbone wouldn't have the fallback so it wouldn't be detected in those libs. If folks really want support there's Symbol-ish shims in es6-shim & core-js.

megawac · 2016-07-21T15:20:33Z

I agree with JDD, only export iterators if Symbol.iterator is present in the environment or shimmed -- I'm not a fan of the @@iterator approach

megawac · 2016-07-21T15:25:50Z

backbone.js

+
+        // Construct a value depending on what kind of values should be iterated.
+        var value;
+        if (this._kind === 'value') {


I think I'd rather define an enum of kinds rather than use strings but not strongly opposed to this

Do you have a preferred enum style in mind? I don't feel strongly about this, happy to take suggestions.

leebyron · 2016-07-21T19:19:14Z

Thanks for the issues, @jdalton that's informative. I don't think those apply here - those issues are about consuming iterables rather than implementing them. The issue there is that ramda was detecting iterables by looking for Symbol.iterator if it exists, but only falling back to "@@iterator" if it didn't - which meant that if an implemented iterable only implemented "@@iterator" that ramda wouldn't detect it was iterable in environments with Symbol defined. The proposed solution in that issue is a good one: to check for either Symbol.iterator or "@@iterator" existence to determine if something is iterable regardless of if Symbol is defined. For example, React does this kind of double-detection in its support of iterables.

For libraries that implement iterable, there's very little downside to using the fallback. Libraries consuming iterables which only look for Symbol.iterator would behave identically when given an Iterable which does or doesn't use the fallback. Older versions of Firefox will suddenly work correctly (which support Iterables but use the pseudo-symbol). Libraries which look for both will benefit from having iterables define the fallback without requiring es6-shim or core-js. The downside is that there is an additional prototype method defined which, as you mentioned, appears if you Object.keys() the prototype.

Anyhow, I can remove it since you both feel strongly about it. Supporting Iterable in ES6 (or shimmed) environments is still great even without using the fallback pseudo-symbol.

leebyron · 2016-07-21T19:29:04Z

backbone.js

+
+  var KIND_VALUE = 1;
+  var KIND_KEY = 2;
+  var KIND_KEY_VALUE = 3;


@megawac let me know if you prefer this enum style over strings

Yep, I prefer this for sure. Mind renaming these ITERATOR_VALUEs, ITERATOR_KEYS, ITERATOR_KEYVALUES

jdalton · 2016-07-21T19:36:59Z

@leebyron

I don't think those apply here - those issues are about consuming iterables rather than implementing them.

Let's avoid the potential headache, user or otherwise, anyway.

Older versions of Firefox will suddenly work correctly (which support Iterables but use the pseudo-symbol).

User adoption of new FF releases is pretty fast. The '@@iterator' property was used from FF27-FF35 which is less than 0.2% in Dec 2015. Even headless runners, like SlimerJS, now have a minimum of FF38.

Anyhow, I can remove it since you both feel strongly about it. Supporting Iterable in ES6 (or shimmed) environments is still great even without using the fallback pseudo-symbol.

Great! As a result of this PR I've updated Lodash's checks to allow shims too.

leebyron · 2016-07-21T19:55:08Z

Great! As a result of this PR I've updated Lodash's checks to allow shims too.

🎉

megawac · 2016-07-21T20:01:45Z

LGTM, I'll defer to another contrib to merge /cc @jridgewell @akre54

megawac · 2016-07-21T20:05:12Z

backbone.js

+          value = model;
+        } else {
+          var id = this._collection.modelId(model.attributes);
+          if (this._kind === ITERATOR_KEYS) {


another option to using these enums to indicate what to place as value would be to have the values, keys, and entries pass a function(model, collection) to define the value. I'm fine with this either way.

akre54 · 2016-07-21T20:23:44Z

Looks great. Thanks @leebyron! I think we're at the point where if you need this you're probably targeting evergreen browsers anyways. We can reassess if anyone feels particularly strongly.

This introduces new methods related to Iterators on Backbone.Collection to mirror those found on Array: `values`, `keys`, `entries`, and `@@iterator`. Each of these methods will return a JavaScript Iterator, which has a `next` method, yielding the models or ids of models contained in the Collection. The CollectionIterator is careful to use the `at()` and `modelId()` methods on the host collection rather than direct access to the `models` property, which should ensure it is resilient to creative subclassing of Backbone.Collection and future feature addition. The [`@@iterator`](http://www.ecma-international.org/ecma-262/6.0/#sec-well-known-symbols) method is defined using `Symbol.iterator` if it exists in the JavaScript runtime (modern browsers/node.js) and falls back to the string `"@@iterator"` which was popularized by older versions of Firefox and has become the standard fallback behavior for other third-party libraries. This ensures that Backbone can still be used across all browsers, even with use of these new methods. Supporting Iterable allows better integration between Backbone and the most recent additions to the JavaScript language, including `for of` loops and data-collection constructor functions, as well as better integration with other third-party libraries that accept Iterables instead of only Arrays. Fixes jashkenas#3954

leebyron · 2016-07-21T20:34:26Z

Comments added, @megawac. Passing functions is maybe overkill, I'm happy with the documented enums.

I agree, @akre54, targeting evergreen browsers is good practice. I'm happy with the state of this PR.

jridgewell · 2016-07-21T21:21:28Z

I dig it. 👍

megawac reviewed Jul 21, 2016
View reviewed changes

leebyron force-pushed the iterable branch from 0c0e0f4 to 30007fc Compare July 21, 2016 19:23

megawac added the enhancement label Jul 21, 2016

leebyron force-pushed the iterable branch from 30007fc to c91603c Compare July 21, 2016 19:28

leebyron reviewed Jul 21, 2016
View reviewed changes

leebyron force-pushed the iterable branch from c91603c to be67b73 Compare July 21, 2016 19:56

megawac reviewed Jul 21, 2016
View reviewed changes

leebyron force-pushed the iterable branch from be67b73 to a2fc65b Compare July 21, 2016 20:33

jridgewell merged commit 96e94b3 into jashkenas:master Jul 21, 2016

leebyron deleted the iterable branch July 22, 2016 18:32

jashkenas added the fixed label Sep 7, 2016

megawac mentioned this pull request Apr 9, 2017

Prepare Backbone 1.4 #4028

Closed

jgonggrijp added a commit that referenced this pull request Feb 25, 2022

Reconcile #3966 with #4061

00f984e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements the JavaScript Iterable protocol. #4061

Implements the JavaScript Iterable protocol. #4061

leebyron commented Jul 20, 2016 •

edited

Loading

jdalton commented Jul 21, 2016 •

edited

Loading

leebyron commented Jul 21, 2016

jdalton commented Jul 21, 2016 •

edited

Loading

megawac commented Jul 21, 2016

megawac Jul 21, 2016 •

edited

Loading

leebyron Jul 21, 2016

leebyron commented Jul 21, 2016 •

edited

Loading

leebyron Jul 21, 2016

megawac Jul 21, 2016

jdalton commented Jul 21, 2016 •

edited

Loading

leebyron commented Jul 21, 2016

megawac commented Jul 21, 2016

megawac Jul 21, 2016

akre54 commented Jul 21, 2016

leebyron commented Jul 21, 2016 •

edited

Loading

jridgewell commented Jul 21, 2016

Implements the JavaScript Iterable protocol. #4061

Implements the JavaScript Iterable protocol. #4061

Conversation

leebyron commented Jul 20, 2016 • edited Loading

jdalton commented Jul 21, 2016 • edited Loading

leebyron commented Jul 21, 2016

jdalton commented Jul 21, 2016 • edited Loading

megawac commented Jul 21, 2016

megawac Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

leebyron Jul 21, 2016

Choose a reason for hiding this comment

leebyron commented Jul 21, 2016 • edited Loading

leebyron Jul 21, 2016

Choose a reason for hiding this comment

megawac Jul 21, 2016

Choose a reason for hiding this comment

jdalton commented Jul 21, 2016 • edited Loading

leebyron commented Jul 21, 2016

megawac commented Jul 21, 2016

megawac Jul 21, 2016

Choose a reason for hiding this comment

akre54 commented Jul 21, 2016

leebyron commented Jul 21, 2016 • edited Loading

jridgewell commented Jul 21, 2016

leebyron commented Jul 20, 2016 •

edited

Loading

jdalton commented Jul 21, 2016 •

edited

Loading

jdalton commented Jul 21, 2016 •

edited

Loading

megawac Jul 21, 2016 •

edited

Loading

leebyron commented Jul 21, 2016 •

edited

Loading

jdalton commented Jul 21, 2016 •

edited

Loading

leebyron commented Jul 21, 2016 •

edited

Loading