ljharb / object.assign

ES6 spec-compliant Object.assign shim. From https://github.com/es-shims/es6-shim
https://tc39.es/ecma262/#sec-object.assign
MIT License
107 stars 22 forks source link

Attempt to feature-detect Symbols, even if `typeof Symbol() !== 'symbol'` #12

Closed ljharb closed 9 years ago

ljharb commented 9 years ago

@webreflection Please test this against your polyfill, and let's discuss any parts that don't pass.

WebReflection commented 9 years ago

no, line 5 is clearly back to "impossible to satisfy" land from a partial polyfill or sham perspective

ljharb commented 9 years ago

Right, that's an early return for environments with native Symbols.

WebReflection commented 9 years ago

I mean Symbols that are polyfilled are threated as strings ... quite inevitably, I'm afraid.

WebReflection commented 9 years ago

here how I'd go:

var obj = {};
var sym = Symbol('test');
obj[sym] = 1;
for (var k in obj) return false;
if (!obj.propertyIsEnumerable(sym)) return false;
if (Object.keys(obj)[0] === sym) return false;
if (Object.getOwnPropertyNames(obj)[0] === sym) return false;
return true;
ljharb commented 9 years ago

Instead of Symbol() returning a string, could it return an Object?

WebReflection commented 9 years ago

it could return a new String as object, the question is: what would that solve since it will always be threated as string property in non compatible engines? If you give me a valid reason, I'll update my poly in seconds.

ljharb commented 9 years ago

The only thing I can think of is that if a user wants to differentiate between Symbol keys and string keys from the key alone, typeof key === 'string' is the only way to do it in a Symbol-polyfilled engine.

WebReflection commented 9 years ago

here the catch: Symbols will never show up in for/in, in Object.keys, or in Object.getOwnPropertyNames ... so, in order to retrieve them, you need to explicitly use Object.getOwnPropertySymbols, meaning of course those are Symbols.

The way you assign or define strings or Symbols is the same, and same is th eway to retrieve descriptors ... this all by specs. So, what's the real-world benefit of having to coerce all Symbols to strings once used as properties if not slowing down all iteractions?

And if you are not fooled by string as typeof, what would object give you instead?

ljharb commented 9 years ago

Array.from(Reflect.enumerate) will provide an array of all string keys and symbols on the entire prototype chain - I'd definitely find it useful to iterate over that array and filter out Symbol keys, or string keys, using typeof === string. You're right though, that there could be a performance hit, but if it's a real String object, will that actually apply? It seems like engines should optimize that case pretty well.

WebReflection commented 9 years ago

btw, in my related post, I've shown a way to know if Symbol works, simplified as such:

var isSymbol = (function (cavy) {
  return function isSymbol(s) {
    switch (typeof s) {
      case 'symbol': return true;
      case 'string':
        cavy[s] = 1;
        var isit = Object.getOwnPropertyNames(cavy).indexOf(s) < 0;
        delete cavy[s];
        return isit;
      default: return false;
    }
  };
}({}));

isSymbol('');       // false
isSymbol(Symbol()); // true
ljharb commented 9 years ago

As far as I can tell, the only polyfill that even closely approaches 100% proper Symbol semantics is https://github.com/WebReflection/get-own-property-symbols - and I'd prefer to make this check be as restrictive as possible, so that your shim is the minimum threshold for acceptability.

WebReflection commented 9 years ago

OK, Array.from(Reflect.enumerate) is a good example of a list that could contain both strings and Symbols.

What I find ridiculous in ES6 is that after all these years of primitives VS instanceof ehnanigans this has landed as false statement:

Symbol() instanceof Symbol

I will update my poly now regardless ... it will take a bit longer than I thought since being a string was actually "a feature"

ljharb commented 9 years ago

instanceof has been a lie for far too long for me to regret it's continual death :-)

Sounds good - I'll leave the string check in then. I've updated the diff; please let me know if that's the only line that fails?

WebReflection commented 9 years ago
make hint
node node_modules/jshint/bin/jshint build/get-own-property-symbols.max.js
build/get-own-property-symbols.max.js: line 97, col 14, Do not use String as a constructor.

1 error
Makefile:72: recipe for target 'hint' failed
make: *** [hint] Error 2

(╯°□°)╯︵ ┻━┻

ljharb commented 9 years ago

lol, true. Object('foo') works too, usually without breaking jshint

WebReflection commented 9 years ago

as future reference: Symbols are shimmed through ES5 descritpors. These are stored as strings, so iif a Symbol is not typeof string it will be that type whenever you retrieve it back via native methods.

Accordingly, Symbol can be polyfilled only as string, unless you hold on memory both the string and its object wrapper counterpart, which is a no-go for anything IoT or low-amount-of-RAM related.

We could all have better Symbol but we cannot have all the platforms supported, so right now my poly will keep the typeof string behavior.

However, there is actually no way to retrieve Symbols if not rhrough modern ES6 methods, so I rather fix all of them as Poly, instead of compromising adoption and performance all over.

Hope this makes sense, latest 0.4.0 version of my poly also fixes other cases like Object.getOwnPropertyDescriptor and its enumerability.

WebReflection commented 9 years ago

rewind

version 0.5.0 actually uses Object("string") as Symbol so that typeof Symbol() === 'object'

This makes filtering by typeof 'string' possible in mixed lists, like the one resulting from Array.from(Reflect.enumerate)

Already published in npm too

https://github.com/WebReflection/get-own-property-symbols/issues/2

ljharb commented 9 years ago

Thanks, updated!

WebReflection commented 9 years ago

you test everything in there ... too bad typeof cannot be overwritten :-)

thanks for making this polyfill compatible with my partial Symbol polyfill attempt.

:+1:

ljharb commented 9 years ago

Published as v3.0.0