Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: jQuery.isFunction returns false for Function objects with custom @@toStringTag #3600

Closed
akihikodaki opened this issue Apr 1, 2017 · 11 comments
Assignees
Labels
Milestone

Comments

@akihikodaki
Copy link

@akihikodaki akihikodaki commented Apr 1, 2017

Description

jQuery.isFunction returns false for Function objects with custom @@toStringTag. That is undesirable in some case such as GeneratorFunction.
This flaw was pointed out at #3597 (comment).

Link to test case

https://jsbin.com/xocanozuye/1/edit?html,js,console

console.log(jQuery.isFunction(function *(){}));
@akihikodaki akihikodaki changed the title Core: jQuery.isFunction returns false for objects inheriting Function objects. Core: jQuery.isFunction returns false for Function objects with custom @@toStringTag Apr 1, 2017
@Krinkle
Copy link
Member

@Krinkle Krinkle commented Apr 1, 2017

@gibson042 From a quick check seems like patching $.type() to align more with typeof should otherwise be harmless. We've already been going in that direction anyways.

As of 3.2, we already do this for most cases. We now only use class2type[toString] for values where typeof returns function or object.

If we remove function from that, reducing use of class2type to object only, should do the trick.

Although we may need to find another way to support Android 2.3's RegExp values in that case.

type: function( obj ) {
	if ( obj == null ) {
		return obj + "";
	}

	// Support: Android <=2.3 only (functionish RegExp)
	return typeof obj === "object" || typeof obj === "function" ?
		class2type[ toString.call( obj ) ] || "object" :
		typeof obj;
},
isFunction: function( obj ) {
	return jQuery.type( obj ) === "function";
},
@mgol
Copy link
Member

@mgol mgol commented Apr 1, 2017

@akihikodaki
Copy link
Author

@akihikodaki akihikodaki commented Apr 2, 2017

That breaks for some cases such as document.createElement( "object" ). It is/was actually a function (callable object), but jQuery.isFunction( document.createElement( "object" ) ) is expected to be false.

	obj = document.createElement( "object" );

	// Firefox says this is a function
assert.ok( !jQuery.isFunction( obj ), "Object Element" );

obj = document.createElement( "object" );

On the other hand, the documentation says:

Determine the internal JavaScript [[Class]] of an object.

https://github.com/jquery/api.jquery.com/blob/eceefcf142534cb51ae443157a4752c9357f3ebd/entries/jQuery.type.xml#L10

Though [[Class]] does no longer exist since ECMAScript 2015, it is similar to @@toStringTag and people may consider jQuery.type derives from that. The change regarding GeneratorFunction as function breaks this expectation.

We need to make clear what we expect for jQuery.isFunction and jQuery.type rather than considering only GeneratorFunction to prevent such confusions.

@gibson042
Copy link
Member

@gibson042 gibson042 commented Apr 2, 2017

I'd rather deprecate jQuery.type than try to keep up with external changes. jQuery.isFunction doesn't need to rely on it, and shouldn't do so any longer IMO.

@gibson042 gibson042 added the Core label Apr 2, 2017
@mgol
Copy link
Member

@mgol mgol commented Apr 2, 2017

@gibson042 if all jQuery.isFunction is going to do is checking if typeof is "function" then we should deprecate it as well.

@gibson042
Copy link
Member

@gibson042 gibson042 commented Apr 2, 2017

I'm fine with deprecating it, but unfortunately it's a little more than just typeof because typeof document.createElement("object") === "function" in Firefox.

@mgol
Copy link
Member

@mgol mgol commented Apr 2, 2017

typeof document.createElement("object") === "function" in Firefox

Not just in Firefox, in Chrome it's "function", too. And in both of them it's changed to "object" in a beta version. I couldn't find any other browser (among these we support) where it's "function" so it will soon stop being an issue. Unless we want to be extra-safe here that nothing DOM-based returns true in jQuery.isFunction.

What's funny is in Chrome it used be "object" in versions <=46.

@akihikodaki
Copy link
Author

@akihikodaki akihikodaki commented Apr 3, 2017

It's a DOM specific problem, so why not check if it is an element in the functions which deals with DOM? It is not reasonable to do that in jQuery.isFunction.

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Apr 3, 2017

I'm with @gibson042 that we should discourage the use of these utility functions and not set the expectation they'll be updated for bizarre usage. If someone passes a generator function to jQuery they're doing something mighty strange. DO NOT TAUNT HAPPY FUN JQUERY!

@Krinkle
Copy link
Member

@Krinkle Krinkle commented Apr 3, 2017

If we plan to deprecate these methods, that means people will eventually need to migrate use to typeof, isArray, and other native checks. In that case I propose we keep them unchanged and deprecate them in their current form. There's little point in improving them further to allow new usage of patterns only to require migration later. It's certainly less risky in terms of back-compat and more predictable / easier to document ([[Class]] / @@toStringTag).

@akihikodaki
Copy link
Author

@akihikodaki akihikodaki commented Apr 4, 2017

There's little point in improving them further to allow new usage of patterns only to require migration later.

Closing this issue.

@akihikodaki akihikodaki closed this Apr 4, 2017
gibson042 added a commit to gibson042/jquery that referenced this issue Apr 8, 2017
@gibson042 gibson042 self-assigned this Apr 24, 2017
@gibson042 gibson042 added this to the 3.3.0 milestone Apr 24, 2017
gibson042 added a commit that referenced this issue Apr 24, 2017
@gibson042 gibson042 mentioned this issue Jan 28, 2018
2 of 4 tasks complete
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.