es-shims / get-own-property-symbols

ES6 Object.getOwnPropertySymbols partial polyfill
MIT License
23 stars 10 forks source link

Identification variable as symbol in the user code #2

Closed zloirock closed 9 years ago

zloirock commented 9 years ago

This library has some problems for this case. Available Object.getOwnPropertySymbols, but it's ugly and slow.

Possible use .constructor property, for example, babel uses it in es6.spec.symbols transformer, but in your code

Symbol().constructor; // String, not Symbol

core-js wraps keys for this case.

WebReflection commented 9 years ago

update this bug has been solved thanks to a different discussion/issue mentioned in https://github.com/ljharb/object.assign/pull/12


core-js is more broken and less compliant than this library ... and it won't allow interoperability.

I don't know what is this bug about so please clarify the problem. Slow? what? where? 'cause strings as Symbols cannot possibly be slower than constant implicit toString() invoke as property.

I'd like more context and real-world concerns on this bug, thank you.

WebReflection commented 9 years ago

just to mention one: if you natively retrieve getOwnPropertyNames those gonna be strings, not objects, so there's no way core-js would solve that without holding in RAM all possyble created Symbols, which flags this as a won't fix. The constructor check is not a real-world problems because to have Symbols you explicitly need to retrieve them via Object.getOwnpropertySymbols so you know these are what you expect to be.

Accordingly, I am not sure I understand what you are trying to say. Please explain with examples, thanks.

WebReflection commented 9 years ago

to add some context: https://github.com/ljharb/object.assign/pull/12#issuecomment-104058338

zloirock commented 9 years ago

core-js is more broken and less compliant than this library

Except that at compatibility with some older browsers.

please clarify the problem

Simple identification variable as symbol, as in title.

solve that without holding in RAM all possyble created Symbols

You library (and core-js) already hold setters in Object.prototype, why not store symbol wrappers? :)

The constructor check is not a real-world problems

It's better than nothing.

Please explain with examples, thanks.

See babel es6.spec.symbols transformer.

WebReflection commented 9 years ago

Except that at compatibility with some older browsers.

not sure I understand ... this has been tested on Android 2, webOS, and other old browsers with partial ES5 support. core-js AFAIK jsut fails in there because it does not consider descriptors problems over inherited getters/setters

It's better than nothing.

foor which case? in Babel you trust typeof thing to eventually be "symbol" which is not the case in terms of interoperability, same is for this poly.

when do you want to use the constructor check and why?

why not store symbol wrappers? :)

Because there are environments without an infinite amount of RAM

http://www.arduino.cc/en/Main/ArduinoBoardYun?from=Main.ArduinoYUN http://www.intel.co.uk/content/www/uk/en/do-it-yourself/edison.html http://www.espruino.com/Pico http://duktape.org

... etc ...

WebReflection commented 9 years ago

also, now that I think about it, and out of curiosity: how are you making it any faster checking and returning wrappers of getOwnPropertyKeys per each property instead of just returning strings?

zloirock commented 9 years ago

You realy think setters size is much lower? :D

also, now that I think about it, and out of curiosity: how are you making it any faster checking and returning wrappers of getOwnPropertyKeys per each property instead of just returning strings?

You can see code, simple hash.

WebReflection commented 9 years ago

simple hash to retrieve per each property in, and each property out ... can I see the benchmark where this polyfill is slower in setting Symbols over N objects? 'cause that's the use case for Symbols, right?

On top of that, yes, wrappers mean memory, if you actually checked my links you would have seen environments with 128kB Flash and 8kB RAM ... I won't solve any problem in there, but why would I potentially hold an infinite amount of wrappers over strings for no real-world benefits whatsoever?

WebReflection commented 9 years ago

to clarify the bench:

var realWorld = Symbol('welcome');

fewObjects.forEach(function (obj) {
  obj[realWorld] = someValue;
});

I wonder how can string wraps perform better than just strings. It's a honest question, since Symbols are used as such, not as properties desriptors or similar.

zloirock commented 9 years ago

slice in your code isn't much faster. You can write tests if you wanna. IIRC, in my tests in most old browsers slowdown for wrapped Object.getOwnPropertyNames was ~4x as compared to native. I came here to point out the problem behavior, and not to argue.

On top of that, yes, wrappers mean memory

Setters also not free.

WebReflection commented 9 years ago

OK, I understand this is not a bug, rather a rant about best practices, with the opportunity to flag it as ugly behind the scene or something.

So, since this just work and is more compliant than current core-js, which brings jsut about nothing better in if not the wrapping object overhead, I'll close until a bug comes up.

WebReflection commented 9 years ago

Available Object.getOwnPropertySymbols, but it's ugly and slow.

... I wonder your meaning of

I came here to point out the problem behavior, and not to argue.

so setters aren't for free, but if shared across all obejcts, are more welcome than a wrapper per each object or each symbol.

You should really come back with an enhancement request and some benchmark that I won't write that demonstartes the following:

  1. you are using less RAM than needed
  2. you are fast at setting and getting properties multiple times over same objects
  3. you are fast at setting and getting properties multiple times over different objects
  4. the final size is smaller and more compatible than this one

I actually do apologies my replies look that harsh and rude, but I reall yhave better things to do these days.

Best Regards

zloirock commented 9 years ago

so setters aren't for free, but if shared across all obejcts, are more welcome than a wrapper per each object or each symbol.

Realy? :) In your code for each symbol we have setter + closure for one symbol, in core-js setter + closure + simple object-wrapper. You can test memory usage, IIRC small differences.

Simply not to report spec compliance issues than see this reaction. Did not expect :D Good luck.

WebReflection commented 9 years ago

you haven't reported any spec cmpliance issue, you have reported a subjective vision of performance.

setter + closure + simple object-wrapper. You can test memory usage, IIRC small differences.

small is relative, I'm pretty sure you know that ... and it's not about assigning objects instead of stings, it's about modifying all methods hat returns Symbols and descriptors to have the riight object wrap.

Do a full spectrum test/perf analysis and you'll find this report kinda lame and a bit pointless for any real-world concern + you'll probably also find worst performance in core-js but again, until somebody that claims better performance show a real-world test, it's just FUD and speculation.

zloirock commented 9 years ago

Symbol.prototype.constructor isn't spec cmpliance issue? :D I'm not going to write or search my old tests, I don't use your library, I just wanted to help to meet the specifications, but looking at your reaction - in vain. As I say above - good luck.

WebReflection commented 9 years ago

Available Object.getOwnPropertySymbols, but it's ugly and slow.

good luck to you, glad I've already closed this!

WebReflection commented 9 years ago

and btw, you didn't help, not at all, so if that was your goal, please re-evaluate the meaning of help. I don't care, maybe other OSS repositories will. Just a personal hint, nothing strictly personal, really

zloirock commented 9 years ago

@WebReflection you do not really understand what it is about the application of the method to identify variable as symbol (the only way in your library)? :D LOL.

and btw, you didn't help

You can revert changes from first issue.

WebReflection commented 9 years ago

to identify variable as symbol

that's by specs typeof variable === "symbol" which by all means, cannot be polyfilled ... how else can I help you?

WebReflection commented 9 years ago

You can revert changes from first issue.

I mean you didn;t help here, thanks for the other bug ... anyway ...

You came here to point out this poly is ugly (what kind of parameter is that? what does it mean) and slow (where is the benchmark?) complaining about constructor and coomparing core-js which has more problems than this partial polyfill.

How am I suppose to see all this as a way to discuss possible enhancements?

If you can tell me what is this bug about, beside ignoring this is a polyfill, as you've done in the other bug where you indeed pointed out real-world problems, I'll be happy to solve the bug for you.

Best Regards

zloirock commented 9 years ago

Your point is clear, can you stop your weird butthurt? :)

You came here to point out this poly is ugly

You can read my previous comment. Or any issue mean "poly is ugly"? :)

WebReflection commented 9 years ago

because of a different problem and a different discussion, your constructor is now Symbol in version 0.5.0 so this is actually a fixed issue.

Any extra, unnecessary, disrespectful comment or insult will result in you blocked here. Please act like a professional, thank you.