Compatibility with core-js symbol sham: Symbol() instanceof Symbol

[zloirock]

Not fixed already near half year :)

[ljharb]

fwiw, I just had a module break because the core-js Symbol shim was being used, and Object.defineProperty didn't work properly with fake Symbols - it's really a horrible idea to use a Symbol sham in production, as it breaks code beyond your own.

I will remove this particular line, and I'll add a comment pointing here. I still think that both of these tests should pass in core-js and in get-own-property-symbols, and I'd prefer to uncomment them in the future. /@WebReflection

I'll update this after I finish adding a test (just like i have for get-own-property-symbols) that covers core-js. The reason it's been half a year is because I've been waiting for you to PR in a test for your particular symbol sham.

[WebReflection]

are we talking about these two lines?

if (typeof sym === 'string') { return false; }
if (sym instanceof Symbol) { return false; }

I agree for the second one but the first one will never possibly pass so ... nothing to do here?

Actually ... my poly passes these two lines so what are we talking about? What shoudl I fix and how your defineProperty operation was affected?

Please let me know, willing to find a solution if possible

[zloirock]

Object.defineProperty didn't work properly with fake Symbols

What do you mean here? Object.defineProperty also wrapped and should work correctly.

it's really a horrible idea to use a Symbol sham in production

Partly agree - possible slowdown and memory leaking with wrong usage. It should be used cautiously. Mainly it's added as the namespace for well-known symbols.

[WebReflection]

Mainly it's added as the namespace for well-known symbols.

agreed ... so, what exactly should (we?) I do in my sham?

I don't see anything different in this thread from what I've done/know already.
If I'm missing something, please let me know.

AFAIK my sham passes already those two checks and my opinion about wrapping/swapping native constructors didn't change: it won't land in any of my code.

I am curious to know in which case defineProperty didn't work 'cause that one is swapped indeed but it's also tested a lot and and never gave me problems.

Regards

[ljharb]

ljharb/global-cache@ba20781 is the fix I had to make using Symbol.for to protect against core-js. I didn't dig into it that deeply, if you can fix the bug that'd be great.

@WebReflection The !(sym instanceof Symbol) check currently passes for you, but fails for core-js. I've left the Object(sym) instanceof Symbol one commented out because I understand that you won't add it, and neither your sham nor core-js passes it. However, I think you should replace Object in your code - I don't know why that's any different than any of the other globals you're wrapping. es6-shim does this with Number and RegExp and it works fine.

[WebReflection]

I'm not wrapping globals and I don't want my code to be order dependent with other shims or shams. Moreover, I've already tested the swapped global approach and it fails for my targets that AFAIK are many more and older than what core.js supports.

[zloirock]

es6-shim does this with Number and RegExp and it works fine.

Not exactly. If you interested, I can show problem cases.

[ljharb]

@WebReflection your sham is already dependent on ordering, because other shims/shams could set up or modify the methods you use. It's obv fine if you don't want to support it in your module.

@zloirock of course, I'd love it if you could file issues on es6-shim about them.

[WebReflection]

I am not replacing functionality so my module is already non order dependent. I am setting stuff configurable like native, my poly is transparent for any other and I would never include in any project of mine a polifyll that simply remove native behaviors instead of enriching them.

In this sense, changing globals is not something I am doing, you are using native globals, not user defined one, and the method I am "overriding" are based on global behaviors too.

So this is less obtrusive, replacing Object would be a disaster like already tested in few targets, which the part you keep ignoring from all my answers.

[zloirock]

@ljharb ok, main problems with Number constructor: paulmillr/es6-shim#365

[ljharb]

@WebReflection you have no guarantee that the Object.defineProperty you're using, for example, hasn't been modified by something that ran before your code. It's fine to say "well just don't include code that does that" but that's not a practical guarantee people have. I hear that you're saying it "would be a disaster" but can you provide specific examples? If you provide a branch on your symbol sham repo, i'll be happy to take a look at it and help.

@zloirock Thanks!

[WebReflection]

you have no guarantee that the Object.defineProperty you're using, for example, hasn't been modified by something that ran before your code.

Yes, I do always know what I include in my projects.

The disaster is about devices failing in any possible way, devices I believe you cannot tests, devices that are targets of my sham ... I haven't said that just for fun, I've wasted time trying before, like months ago, and I am not willing to fix this but I don't mind if you'll drop support.

This is the list: https://github.com/WebReflection/get-own-property-symbols#compatibility

[WebReflection]

FWIW what I've done in order to not compromise native functionality but being easily able to distinguish between strings, objects, and Symbols, version 0.6.1 returns '[object Symbol]' when Object.prototype.toString.call(sym) is used against a Symbol.

Since typeof cannot be overridden, I think this would be the best way to distinguish them in a "shammed" environment.

[zloirock]

IIRC it's in core-js from the last year as a part of @@toStringTag logic :)

[WebReflection]

yeah, I need to better implement that part too but I don't see here any check against this behavior which is actually the best to recognize symbols in polyfilled/shammed engines.

I might have a look at what you've done in toStringTag and borrow it :P