Skip to content

Commit

Permalink
Symbol() should be a primitive by spec, and should not be `instance…
Browse files Browse the repository at this point in the history
…of Symbol`
  • Loading branch information
ljharb committed May 21, 2015
1 parent 980d23d commit 59b94e0
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions hasSymbols.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ module.exports = function hasSymbols() {
var obj = {};
var sym = Symbol('test');
if (typeof sym === 'string') { return false; }
if (sym instanceof Symbol) { return false; }

This comment has been minimized.

Copy link
@zloirock

zloirock May 21, 2015

Interesting :) It added for disable support core-js? Why not added also Object(Symbol()) instanceof Symbol? :)

This comment has been minimized.

Copy link
@ljharb

ljharb May 21, 2015

Author Owner

I'm planning on adding that next - I'm using https://github.com/WebReflection/get-own-property-symbols as my metric, and I opened es-shims/get-own-property-symbols#4 for the Object() case.

Can core-js fix itself so this test passes?

If you want to submit a PR that adds a test script, similar to the one I'm using for get-own-property-symbols, for core-js, so I can run its tests too, that would help ensure I don't knowingly exclude core-js as I inevitably tighten these checks over time.

This comment has been minimized.

Copy link
@zloirock

zloirock May 22, 2015

core-js can fix it, but I don't think it's a good idea, zloirock/core-js#16. It creates more problems than benefits. For example, someone extends Symbol.prototype (already seen this in practice) and it will not work after this change. Other tests passes without any problems.

This comment has been minimized.

Copy link
@ljharb

ljharb May 22, 2015

Author Owner

The Symbol() object can still have Symbol.prototype and not be instanceof Symbol - I'm not sure why that would be a problem?

This comment has been minimized.

Copy link
@zloirock

zloirock May 22, 2015

Show me how :) I see only one ugly way. Anyway, it's not the only problem.

This comment has been minimized.

Copy link
@ljharb

ljharb May 22, 2015

Author Owner

It may indeed be ugly. I'll play with it and see what I can come up with.

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 21, 2015

Author Owner

Linking back to #17

obj[sym] = 42;
for (sym in obj) { return false; }
if (keys(obj).length !== 0) { return false; }
Expand Down

0 comments on commit 59b94e0

Please sign in to comment.