Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

I think this is a good time for some review. Is there a reason for jQuery.type to return "symbol"? On the one hand yes, it's a new type, but on the other why is a Symbol being passed to anything that jQuery is handling since we never do anything with it? If we are going to make this change I think it would be good to also take a look at all our other type-sniffing methods like isPlainObject and isNumeric to see what they do with Symbol as well.

We never do anything with it but jQuery.type is a public API so it should be accurate. I think this is a good thing to fix, especially that the patch is small.

@dmethvin

var s1 = Symbol('id');
var s2 = new Object(s1);

$.isPlainObject(s1); // false
$.isPlainObject(s2); // false

$.isEmptyObject(s1); // true
$.isEmptyObject(s2); // true
// But also:
$.isEmptyObject(42); // true

$.isNumeric(s1); // throws on parseFloat(s1)
$.isNumeric(s2); // throws on parseFloat(s2)

Throwing in $.isNumeric can be prevented by changing parseFloat( obj ) to parseFloat( String( obj ) ).

@mzgol in what browsers do you have this result?

I tested in Chrome but those methods are generic enough it should work in
the same way in every browser supporting symbols. Do you have different
results somewhere?

Michał Gołębiowski

Any new ES entity might be implemented across all vendors with some subtle differences, violating the spec or exploiting some undefined behaviours (that happens a lot). Whereas right now, most code that uses ES6, probably run through transpiler, whereas transpiler might or not might use a shim (babel, for example, does use it for Symbol).

So in order to be sure in consistent results you shown above, we need to properly research it, checking it in any possible environment.

@markelog Good point but let see what exactly happens there according to the spec instead of real implementations:

1. $.isPlainObject returns false because for both boxed & unboxed symbols s.constructor is the global Symbol; they behave the same because primitives are auto-boxed on property access. For the latter, as http://www.ecma-international.org/ecma-262/6.0/#sec-objects specifies, every object has a constructor field pointing to the constructor; as http://www.ecma-international.org/ecma-262/6.0/#sec-symbol-constructor specifies, the Symbol function creates an instance of the Symbol value so it matches.
2. $.isEmptyObject returns true because it returns false only inside the loop for ( name in obj ) { return false }, symbols don't have any string-keyed enumerable properties and every primitive is converted to an object when iterated via for..in (http://www.ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-forin-div-ofheadevaluation-tdznames-expr-iterationkind, point 7.b.).
3. $.isNumeric throws because it invokes parseFloat on a symbol and:
   1. parseFloat converts its parameter to string via the abstract ToString operation (http://www.ecma-international.org/ecma-262/6.0/#sec-parsefloat-string, point 1)
   2. ToString on a symbol throws a TypeError according to the table at http://www.ecma-international.org/ecma-262/6.0/#sec-tostring.

So my previous comment was correct anyway.

Mostly, i meant that bit -

violating the spec

and

babel, for example, does use it for Symbol

That happend before, like with v8 initial implementation of promises

Yeah, but considering that Symbols are not in IE, other browsers are mostly evergreen so they are patched regularly and jQuery doesn't use Symbols internally I don't think we should fix any possible Symbol-related bugs in Core, we should just make sure we behave correctly for standards-based implementations. We might make an exception for Babel/Traceur but I wouldn't care too much about real environments here.

FWIW, Node 0.12 & 4.0 works as Chrome 45 with these functions and Node 0.10 doesn't support Symbols.

@mzgol if everything that browsers did would be according to the specification, we could have remove all those evergreen hacks

@markelog Yes, but see the part:

jQuery doesn't use Symbols internally

We don't fix IndexedDB bugs inside jQuery, for the same reason I don't think we should fix any potential Symbol-related bugs.

EDIT: Anyway, if someone finds any Symbol-related bug in a real env that would affect us, let's talk concretely. I doubt such a thing exists so I don't feel compelled to research.

We don't fix IndexedDB bugs inside jQuery

I don't understand the reference

for the same reason I don't think we should fix any potential Symbol-related bugs.

That means we don't support Symbol

Oh well, these tests are simple so I just run them:

var s1 = Symbol('id');
var s2 = new Object(s1);

console.assert($.isPlainObject(s1) === false);
console.assert($.isPlainObject(s2) === false);

console.assert($.isEmptyObject(s1) === true);
console.assert($.isEmptyObject(s2) === true);
// But also:
console.assert($.isEmptyObject(42) === true);

try { $.isNumeric(s1); console.assert(false); } catch (e) {}
try { $.isNumeric(s2); console.assert(false); } catch (e) {}

It passes in Chrome 45, Firefox 41, Edge 12 & Safari 9. Safari < 9 doesn't support Symbol. We can add proper tests for those things, I guess.

We can add proper tests for those things, I guess.

That would be cool to have, yeah, if they would fail, we can open discussion of whenever or not we should be fixing potential bugs. I think we should also try to run them in popular shims too.

Well... The discussion is whether or not to implement this feature and the argument against it is that it is not used by jQuery internally (or that the average jQuery user doesn’t do anything with symbols), right?

@ChristianGrete nuh, we just being careful here, discussing specifics of the implementation and possible risks. We all agree that this should be implemented

@mzgol would you mind adding your tests with follow-up commit?

@mzgol would you mind adding your tests with follow-up commit?

Sure. Should I land this PR then?

Well, @timmywil added Needs review and didn't remove it, also, did you try run them with shim?

Not yet, I won't have time for that before the Summit.

I'm fine with landing this before we add more tests, but we need an issue to track that.

It would be cool to test in the shim, just to be sure

Fair enough.

@markelog I assume you mean to test Babel with the core-js shim? In that case I'd like to first fix the test at test/node_smoke_tests/iterable_with_symbol_polyfill.js to work in browsers (this will allow us to update jsdom) and then adding a similar test for this issue will be simple. The initial work is not a one-liner, though, so I won't have time to do it before the summit.

@mzgol i'd say your conformation that this is works is enough, after that i can create ticket like @timmywil suggested and track everything else from there

I won't be 100% sure if it works before I test it. ;) That said, this reminded me that unless you enable the non-default es6.spec-symbols transformer typeof isn't shimmed by Babel so for the majority of users typeof Symbol('id') will return object; try e.g. this repl in IE11.

I won't be 100% sure if it works before I test it

I meant you could just test it locally, without adding anything to the dist, conform that it is works and we could deal with everything else later - that would be enough for me.

It's not that simple; I'd have to be careful in checking if I'm testing the right thing, there's also the issue about typeof not being shimmed by default which I described above. I don't have time now and I don't want to rush it.

about typeof not being shimmed

Transformed, not shimmed, otherwise we will be lost in terminology :-), when you use babel shim collection, you would need to use such transforms, since shims are not included by default, i assume that is reason why such transforms are not included too.

So it should be pretty easy, but i guess we can wait :-)

The Babel's idea is that some transformers fix edge cases and are enough non-performant that the majority of users won't need them. They are marked with the es6.spec. prefix & disabled by default. I don't have data about it but I assume most people will run Babel with default settings, adding only the browser-polyfill.js file (which contains compiled core-js) or enabling Babel runtime which adds it dynamically (at least that's what I see in most projects I look at). Therefore, most Babel users won't have typeof transpiled to be ES6-compliant so this patch won't work for them in non-modern browsers.

I'm not sure if we should care about it but that's how it is.

In any case, that is not our problem, if some shim requires some deps, it is their issue, we can do only so much, so i would suggest to check correctness with full version

OK, I was wrong, since jQuery.type in no way uses typeof, the non-compliant default typeof Symbol treating is not a problem.

The patch makes it work as seen in this simple test case (just open it with a browser not supporting ES6 Symbol as IE 11).

@mzgol sounds like you confirmed it :-)
@ChristianGrete would you mind taking in the consideration my suggestion? After that, i think we all set!

Okay, i moved that test for you, thank you!

@markelog Of course not, I’m totally fine with that. Just wasn’t sure where to properly put the test, so I added it there. 😉