Core: Make jQuery objects iterable #2369

mgol · 2015-06-01T21:26:46Z

Make iterating over jQuery objects possible using ES 2015 for-of:

for ( node of $( "<div id=narwhal>" ) ) {
    console.log( node.id ); // "narwhal"
}

Fixes gh-1693

TODO:

Add a Node.js test checking if the iterator is defined even for core-js-polyfilled Symbol & Symbol.iterator.
Add tests ensuring that the iterator works with babel + core-js polyfills.
Rename jsHint to JSHint in comments.
Drop the Symbol.iterator check, leave just the typeof Symbol one.
Switch [][Symbol.iterator] to arr[Symbol.iterator].
~~Use QUnit.skip to skip the test in non-ES6 browsers.~~ Just use the ok assertions in non-ES6 browsers.
Split the JSHint upgrade to a separate commit (to do just before landing).

@rwaldron, could you have a look if it's safe to include this in 3.0.0?

scottgonzalez · 2015-06-01T21:29:19Z

src/core.js

+// unguarded in another place, it seems safer to just disable jsHint for these
+// three lines.
+/* jshint ignore: start */
+if ( typeof Symbol === "function" && Symbol.iterator ) {


Why not just check for Symbol on the global instead of directly as a global?

Because we don't have a reference to the global, we only have reference to window. In the browser they're equivalent but we also support running jQuery on Node (in a very basic way but still) where window is a fake window object created by a library like jsdom.

To reference a global we'd need to pass an additional parameter to the jQuery factory... but that wouldn't work since if used with Browserify this references the module and not the global. So we'd basically need to run a cascade test - check for the global variable, fallback to this. This wouldn't be shorter and would certainly be more fragile.

Thanks. I didn't realize we didn't have a reference to the global.

Should this check typeof Symbol.iterator === 'symbol'?

Do we need to worry about cases where a global Symbol is defined as a
function and Symbol.iterator doesn't have a typeof equal to "symbol"? I
imagine this would break using this code with Symbol polyfills that can't
override typeof.

Michał Gołębiowski

and advocate for letting jQuery be exactly as iterable as arrays sharing its environment (and hence not to check for Symbol.iterator at all).

Almost agree. If there were an environment that implemented Symbol, but hadn't implemented Symbol.iterator yet, you'd end up with jQuery.fn.undefined = undefined. If you don't care about that, then it's all good and I agree. Also, I don't know of any runtimes that match that, but for a few months both Chrome and Firefox did (obviously not anymore).

If there were an environment that implemented Symbol, but hadn't implemented Symbol.iterator yet, you'd end up with jQuery.fn.undefined = undefined.

Correct. Or perhaps even something a little weirder, if Symbol.iterator is partially implemented (e.g., by incomplete polyfill).

If you don't care about that, then it's all good and I agree.

Correct. I don't care about that, and here's why (edit mine):

I don't know of any runtimes [supported by jQuery] that match that

Current situation is as follows:

Latest stable Firefox, Chrome & Opera support Symbol, Symbol.iterator & for-of on any collections.

IE11 supports neither Symbol nor for-of but Edge supports both.

Safari 8 doesn't support Symbol; it supports for-of only on Arrays. WebKit Nightly supports Symbol, Symbol.iterator & for-of on any collections.

I don't think we're in a lot of danger that any browser will arrive with that much broken Symbol global judging by the above info.

@mzgol as always, thank you for the thorough follow up

rwaldron · 2015-06-02T14:21:33Z

👏 This is so awesome.

gibson042 · 2015-06-02T15:25:36Z

src/core.js

+// three lines.
+/* jshint ignore: start */
+if ( typeof Symbol === "function" && Symbol.iterator ) {
+	jQuery.fn[ Symbol.iterator ] = [][ Symbol.iterator ];


This should use arr instead of [].

mgol · 2015-06-04T13:33:47Z

PR updated, please re-review.

mgol · 2015-06-04T13:35:40Z

.gitignore

@@ -10,3 +10,5 @@

 /dist
 /node_modules
+
+/test/node_smoke_tests/lib/ensure_iterability.js


I could put it all in dist and not add anything to .gitignore or .jshintignore but then I'd have to copy the entire test/node_smoke_tests directory over as the ensure_iterability_es6.js file requires files relative to its original location so transpiling to a different directory wouldn't fly.

mgol · 2015-06-04T13:37:57Z

package.json

    "grunt-cli": "0.1.13",
    "grunt-compare-size": "0.4.0",
-    "grunt-contrib-jshint": "0.10.0",
+    "grunt-contrib-jshint": "0.11.2",


This is required so that typeof x === "symbol" works in esnext mode.

gibson042 · 2015-06-04T13:43:31Z

test/node_smoke_tests/lib/ensure_iterability_es6.js

+
+		if ( result !== "DIVSPANA" ) {
+			console.error( "for-of doesn't work on jQuery objects; " +
+				"expected: 'DIVSPANA'; got: '" + result + "'" );


We should probably use assert, but I'm fine with a later sweep of node_smoke_tests for that.

gibson042 · 2015-06-04T13:45:34Z

LGTM 👍

mgol · 2015-06-08T16:43:41Z

I'll land this shortly barring any new remarks.

Utilize the assert module, avoid inline JSHint comments.

Make iterating over jQuery objects possible using ES 2015 for-of: for ( node of $( "<div id=narwhal>" ) ) { console.log( node.id ); // "narwhal" } Fixes jquerygh-1693

jquerybot added the CLA: Valid label Jun 1, 2015

mgol added the Needs review label Jun 1, 2015

mgol mentioned this pull request Jun 1, 2015

Make jQuery collections for-of-able #1693

Closed

scottgonzalez reviewed Jun 1, 2015
View reviewed changes

gibson042 reviewed Jun 2, 2015
View reviewed changes

mgol mentioned this pull request Jun 4, 2015

"symbol" is not recognized as a valid typeof value jshint/jshint#2444

Closed

mgol force-pushed the for-of branch from f255781 to ed1fdb7 Compare June 4, 2015 13:33

mgol reviewed Jun 4, 2015
View reviewed changes

mgol force-pushed the for-of branch from ed1fdb7 to 5b57875 Compare June 4, 2015 13:37

mgol reviewed Jun 4, 2015
View reviewed changes

mgol force-pushed the for-of branch from 5b57875 to fa3059d Compare June 4, 2015 13:39

gibson042 reviewed Jun 4, 2015
View reviewed changes

mgol added 3 commits June 13, 2015 22:45

Build: Update grunt-contrib-jshint

1556c46

Build: Refactor Node smoke tests

9c8a3ec

Utilize the assert module, avoid inline JSHint comments.

Core: Make jQuery objects iterable

bb026fc

Make iterating over jQuery objects possible using ES 2015 for-of: for ( node of $( "<div id=narwhal>" ) ) { console.log( node.id ); // "narwhal" } Fixes jquerygh-1693

mgol force-pushed the for-of branch from fa3059d to bb026fc Compare June 13, 2015 21:17

mgol merged commit bb026fc into jquery:master Jun 13, 2015

mgol deleted the for-of branch June 13, 2015 21:20

mgol removed the Needs review label Jul 31, 2015

andyli mentioned this pull request Sep 8, 2015

New JQuery issues HaxeFoundation/haxe#4528

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Make jQuery objects iterable #2369

Core: Make jQuery objects iterable #2369

mgol commented Jun 1, 2015

scottgonzalez Jun 1, 2015

mgol Jun 1, 2015

scottgonzalez Jun 1, 2015

ljharb Jun 2, 2015

mgol Jun 2, 2015

rwaldron Jun 2, 2015

gibson042 Jun 2, 2015

rwaldron Jun 2, 2015

mgol Jun 2, 2015

rwaldron Jun 3, 2015

rwaldron commented Jun 2, 2015

gibson042 Jun 2, 2015

mgol commented Jun 4, 2015

mgol Jun 4, 2015

mgol Jun 4, 2015

gibson042 Jun 4, 2015

gibson042 commented Jun 4, 2015

mgol commented Jun 8, 2015

Core: Make jQuery objects iterable #2369

Core: Make jQuery objects iterable #2369

Conversation

mgol commented Jun 1, 2015

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

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

rwaldron commented Jun 2, 2015

Choose a reason for hiding this comment

mgol commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Jun 4, 2015

mgol commented Jun 8, 2015