Core: Update isFunction to handle unusual-@@toStringTag input #3617

gibson042 · 2017-04-08T03:30:02Z

Summary

Generators and async functions are still functions (cf. ECMAScript Language Specification), and we can recognize them with typeof (but still need a guard to reject elements).

Fixes gh-3600
Fixes gh-3596

Checklist

All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

…nction This change allows detection of functions with unusual @@toStringTag values.

Ref jquerygh-3597

Fixes jquerygh-3600 Fixes jquerygh-3596

mention-bot · 2017-04-08T03:30:04Z

@gibson042, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @mgol and @jeresig to be potential reviewers.

markelog · 2017-04-08T21:57:17Z

test/unit/core.js

+		doc.write( "<body onload='window.parent.iframeDone( function() {} );'>" );
+		doc.close();
+	} catch ( e ) {
+		window.iframeDone( function() {}, "iframes not supported" );


How this could happen?

The block was patterned off a similar one for isPlainObject, and is quite old as it turns out. I'll remove the try/catch.

markelog · 2017-04-08T22:02:13Z

test/unit/core.js

@@ -489,8 +493,56 @@ QUnit.test( "isFunction", function( assert ) {
 	callme( function() {
 		callme( function() {} );
 	} );
+
+	// Functions from other windows should be matched
+	Globals.register( "iframeDone" );


Can we move this to separate test as well?

I guess, but it seems to fit with the rest of these assertions pretty well. Would anyone else like to weigh in?

markelog · 2017-04-08T22:02:36Z

test/unit/deferred.js

@@ -537,6 +539,14 @@ QUnit.test( "jQuery.Deferred.then - spec compatibility", function( assert ) {
 		assert.ok( true, "errors in .done callbacks don't stop .then handlers" );
 	} );

+	function faker() {


Separate test?

markelog · 2017-04-08T22:02:54Z

test/unit/deferred.js

@@ -861,7 +871,19 @@ QUnit.test( "jQuery.when(nonThenable) - like Promise.resolve", function( assert
 QUnit.test( "jQuery.when(thenable) - like Promise.resolve", function( assert ) {
 	"use strict";

-	var CASES = 16,
+	function customToStringThen() {


Separate test?

I'd really rather not; this test runs four assertions per input thenable.

markelog · 2017-04-08T22:03:54Z

src/core.js

+
+		// Support: Chrome <=57, Firefox <=52
+		// Don't classify callable <object>s as functions
+		return typeof obj === "function" && typeof obj.nodeType !== "number";


typeof obj.nodeType !== "number", what is this? Which test is related to this change?

It's preexisting. This nodeType guard protects us against what would would otherwise be a regression caused by changing from jQuery.type to typeof.

Would you mind adding this clarification comment there, right now it looks so bizarre :)

It reads as Don't classify callable <object> elements as functions now; what would clarify further?

If <object> is referenced to typeof result for objects, how it could bypass out initial typeof obj === "function"?

Then function type is a subtype of object so why we can't classify function object as function?
Most of all, why we check presence of nodeType property to see if object is callable and why do we care?

"elements" part provides some clarification, but only if you know that this is the reference to the DOM elements. And even so why "element"? Why not "node"? If the logic is meant only for elements indeed then how come it cannot reach this part for other nodes? I think it also deserves some explanation as well as some examples of such an input.

I think I know answers to these questions, but after some months I might forget, you might forget, and others who going to read this might not know of course

"Don't classify callable <object> elements as functions" is perfectly clear to me. Can you just tell me what you want to see here?

markelog · 2017-04-10T08:05:57Z

Can we augment logic of jQuery.type instead? like jQuery.type(function *() {}) === 'object' right now.

markelog · 2017-04-10T08:11:56Z

On tests perspective:

In my opinion: one feature – one test, one regression – one test for regression (i.e. for bug), preferably with issue number in test description.

The simpler the better, microservices or docker containers analogies come to mind since otherwise It's really hard to support monolith and god methods with potential merge conflicts if/when we would have to revert if those constructions were already changed at the time of revert. For the same reason we no longer put HTML fixtures inside the test suite, but create them inside the test.

I think we already discuss this many times?

One issue, one pull request, independent tests, one atomic change, one commit

timmywil · 2017-04-10T15:27:03Z

I don't think it is necessary that pull requests have one commit, but I agree with atomicity of commits. As for this particular case, I wouldn't mind rebasing all 4 of these commits into one as they all seem to address the same problem. Still, this is less important than the "how". How we address the problem is always the bigger question, and I'm wondering if we shouldn't align with Lodash here and treat generator functions as functions as well.

gibson042 · 2017-04-10T16:07:50Z

jQuery.type is so vague and generic that I don't want to touch it... we only use it internally for distinguishing number/string/function/object, and even that is potentially problematic as the list of special-cased objects grows. jQuery.isFunction, on the other hand, is much more clear (but broken until this PR lands).

As for testing, I really like the efficiency of table-driven tests, and want to see us move towards them where practical, rather than away. And merge conflicts wouldn't be an issue if we set expectations from the list of cases.

gibson042 · 2017-04-10T17:29:55Z

Tests updated. This should be good to go.

mgol · 2017-04-11T10:14:28Z

src/core.js

@@ -212,7 +212,10 @@ jQuery.extend( {
 	noop: function() {},

 	isFunction: function( obj ) {
-		return jQuery.type( obj ) === "function";
+
+		// Support: Chrome <=57, Firefox <=52


Should we add only to both or not yet because Chrome 58 & Firefox 53 are not stable yet?

Not yet; we can update this after official releases if anyone cares.

mgol · 2017-04-11T10:17:49Z

test/unit/deferred.js

+		function faker() {
+			assert.ok( true, "handler with non-'Function' @@toStringTag gets invoked" );
+		}
+		faker[ typeof Symbol === "function" && Symbol.toStringTag ] = "String";


Why not wrap the statement with if? In the current way we'll be writing to a "false" property in non-supporting browsers which may create confusion.

These are tests so even size is not important so I don't see the gains of the current form.

mgol · 2017-04-11T10:18:11Z

test/unit/deferred.js

+				onFulfilled();
+			}
+		};
+		promise.then[ typeof Symbol === "function" && Symbol.toStringTag ] = "String";


Similarly, I'd prefer an if instead of writing to a "false" property.

Fair enough; updated.

mgol · 2017-04-11T16:19:19Z

test/unit/deferred.js

+			onFulfilled();
+		}
+	};
+	if ( typeof Symbol === "function" ) {


This check is not enough, many browsers (e.g. old Firefox, Edge <15) support many symbols but not @@toStringTag.

This applies to here & a few other places.

I don't think we care about those cases because they coincide with impossibility of setting @@toStringTag, but I'll see what I can do anyway.

I decided to keep this one for its harmless property set, but the others are updated to skip tests unless Symbol.toStringTag is truthy.

markelog · 2017-04-11T11:26:57Z

src/core.js

@@ -464,7 +467,7 @@ function isArrayLike( obj ) {
 	var length = !!obj && "length" in obj && obj.length,
 		type = jQuery.type( obj );

-	if ( type === "function" || jQuery.isWindow( obj ) ) {
+	if ( jQuery.isFunction( obj ) || jQuery.isWindow( obj ) ) {


Nice, did we already deprecated isWindow btw?

Doesn't look like it: https://github.com/jquery/jquery/issues?q=is%3Aissue%20isWindow

Created – #3629 :)

markelog · 2017-04-17T17:03:17Z

src/core.js

+
+		// Support: Chrome <=57, Firefox <=52
+		// Don't classify callable <object>s as functions
+		return typeof obj === "function" && typeof obj.nodeType !== "number";


gibson042 · 2017-04-22T19:19:26Z

src/core.js

-		return jQuery.type( obj ) === "function";
+
+		// Support: Chrome <=57, Firefox <=52
+		// Don't classify callable <object> elements as functions


@markelog ping... what do you want instead of this text?

Sorry, long week. Some in the lines of

// Browsers mentioned above will erroneously equate `typeof` // call on some of the DOM nodes to "function": // // jQuery.isFunction(document.createElement("object")) === true // // but not // // jQuery.isFunction(document.createElement("div")) !== true // // To circumvent it we check presense of correct `nodeType` property on // provided argument, which all DOM nodes shoud have

markelog · 2017-04-23T17:15:25Z

test/unit/core.js

@@ -439,6 +439,8 @@ QUnit.test( "isFunction", function( assert ) {
 	fn = function() {};
 	assert.ok( jQuery.isFunction( fn ), "Normal Function" );

+	assert.ok( !jQuery.isFunction( Object.create( fn ) ), "custom Function subclass" );
+
 	obj = document.createElement( "object" );

 	// Firefox says this is a function


Chrome does the same, correct?

markelog · 2017-04-23T17:30:18Z

test/unit/core.js

@@ -439,6 +439,8 @@ QUnit.test( "isFunction", function( assert ) {
 	fn = function() {};
 	assert.ok( jQuery.isFunction( fn ), "Normal Function" );

+	assert.ok( !jQuery.isFunction( Object.create( fn ) ), "custom Function subclass" );


We can use notOk here instead? It seems more explicit that way, nitpick of course

akihikodaki and others added 4 commits April 7, 2017 22:15

Core: Use instanceof and typeof instead of jQuery.type in jQuery.isFu…

affc1c8

…nction This change allows detection of functions with unusual @@toStringTag values.

Core: Improve unusual-@@toStringTag isFunction tests

4bb72c1

Deferred: Test functions with custom @@toStringTag values

01d85e6

Ref jquerygh-3597

Core: Update isFunction to handle unusual-@@toStringTag input

e4539b4

Fixes jquerygh-3600 Fixes jquerygh-3596

markelog requested changes Apr 8, 2017

View reviewed changes

Tests: Remove unnecessary try/catch

7046d39

Core: Update isArrayLike to reject _all_ functions

1c4725f

Tests: Improve specialization

a254483

mgol reviewed Apr 11, 2017

View reviewed changes

mgol requested changes Apr 11, 2017

View reviewed changes

Tests: Use more explicit Symbol-dependent constructions

1226843

mgol requested changes Apr 11, 2017

View reviewed changes

Tests: Check for Symbol.toStringTag before using it

2444c1d

mgol approved these changes Apr 11, 2017

View reviewed changes

timmywil added this to the 3.3.0 milestone Apr 17, 2017

markelog reviewed Apr 17, 2017

View reviewed changes

Core: Improve isFunction support comment

8e195ed

Florian-R mentioned this pull request Apr 18, 2017

Can't use async functions in callbacks #3631

Closed

yumetodo mentioned this pull request Apr 22, 2017

関数宣言、関数式、Arrow Function asciidwango/js-primer#20

Closed

3 tasks

gibson042 commented Apr 22, 2017

View reviewed changes

markelog requested changes Apr 23, 2017

View reviewed changes

markelog reviewed Apr 23, 2017

View reviewed changes

Core: Update isFunction comments per code review

439133f

gibson042 merged commit a16339b into jquery:master Apr 24, 2017

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

Core: Update isFunction to handle unusual-@@toStringTag input #3617

Core: Update isFunction to handle unusual-@@toStringTag input #3617

Conversation

gibson042 commented Apr 8, 2017

Summary

Checklist

mention-bot commented Apr 8, 2017

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

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

gibson042 Apr 18, 2017 • edited

Choose a reason for hiding this comment

markelog commented Apr 10, 2017

markelog commented Apr 10, 2017 • edited

timmywil commented Apr 10, 2017

gibson042 commented Apr 10, 2017 • edited

gibson042 commented Apr 10, 2017

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

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

gibson042 Apr 18, 2017 •

edited

markelog commented Apr 10, 2017 •

edited

gibson042 commented Apr 10, 2017 •

edited