paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 387 forks source link

"bad map" error when used with `es6-symbol` sham #339

Closed Jamtis closed 9 years ago

Jamtis commented 9 years ago

Hi folks,

on Nodejs v0.10.33 I get the following error:

> require("es6-symbol/implement")
{}
> Symbol
[Function: Symbol]
> require("es6-shim")
TypeError: bad map
    at new Map (/home/codio/workspace/node_modules/es6-shim/es6-shim.js:1974:19)
    at Map (/home/codio/workspace/node_modules/es6-shim/es6-shim.js:2326:19)
    at /home/codio/workspace/node_modules/es6-shim/es6-shim.js:2347:15
    at not (/home/codio/workspace/node_modules/es6-shim/es6-shim.js:22:22)
    at Object.<anonymous> (/home/codio/workspace/node_modules/es6-shim/es6-shim.js:27:2)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)

Is this a fault of mine? If so how do I implement it correctly?

ljharb commented 9 years ago

Pretty sure it's not your fault :-)

Note, however, that Symbols are not actually shimmable without caveats in an engine that doesn't have them natively - so all bets are off once you've attempted to do that.

require('es6-shim') works fine in node 0.10. What happens if you require the es6 shim first, before es6-symbol?

Jamtis commented 9 years ago

Thanks for the quick reply. When I require "es6-shim" first I don't get errors but [Symbol.Iterator] of map instances is undefined which is exactly what I need. Is "es6-shim" capable of shimming [Symbol.Iterator] in maps?

ljharb commented 9 years ago

Because Symbols are unshimmable (and anything attempting to shim them is violating parts of the JS spec), es6-shim in an environment without Symbols uses the arbitrary '_es6-shim iterator_' property ('@@iterator' in some engines) to store its default iterators.

Why do you specifically need Symbol.iterator in an environment without for..of?

Jamtis commented 9 years ago

I use babel to transpile ES6-code containing for..of loops. Babel transpiles it to ES5-code that uses Symbol.iterator. I agree with the consideration of violating the JS spec. So is it a neccessary to have Symbols to iterate over iterables in general or is it babel that's wrong by using Symbol.iterator?

ljharb commented 9 years ago

imo, it's babel that's wrong for attempting to shim Symbols in this way. However, I believe you can specify that in the options, or with a babel transform? Also, if you don't use core-js or es6-symbol, it won't try to shim Symbol, so I'd think babel can transpile Symbol code to something else?

Jamtis commented 9 years ago

By default babel does not shim Symbols but use it in for..of loop transpilations, I didn't find a way to avoid using Symbols. With babel/polyfill (what I use now) Symbols can be shimmed. It's not a good soluation because it violates the JS specs but better than non-working code. :smile:

es6-shim shall not try to shim Symbols which is right. But whats the cause of the "bad map" error? If either no Symbols or native Symbols are present in the global context it works just fine. Why do shimmed Symbols break it?

ljharb commented 9 years ago

Our Map construction code path uses Symbol.species, but if it's not available at require time, it uses the string "@@species". The es6-shim wasn't written to take advantage of non-native Symbols, so it's likely that this interaction is where the problem lies.

Oddly enough, Object.getOwnPropertySymbols is not shimmed by es6-symbol.

ljharb commented 9 years ago

OK, so - it looks like Object.prototype[Symbol.species] is set to undefined by es6-symbol, which means Symbol.species in Map returns true, and so es6-shim's internal defineProperty check refuses to overwrite it.

Thus, while this is a specific use case that es6-shim could hack around, the reality is that es6-symbol is putting properties on Object.prototype which make valid in checks return false positives.

Jamtis commented 9 years ago

Okay, if I got it right es6-symbol is not spec-compilant (besides privacy). Therefore es6-shim crashes due to illegal properties?

ljharb commented 9 years ago

Yes - specifically, Symbol.species in Map returns true because Object.prototype.hasOwnProperty(Symbol.species) returns true, so Map[Symbol.species] ends up not being polyfilled correctly, so new Map throws since it attempts to use Map[Symbol.species].

Jamtis commented 9 years ago

Wouldn't it be better anyway to use an actual value comparison instead of an in-check?

ljharb commented 9 years ago

I don't think so - if someone has assigned undefined to a property, that's an explicit choice we don't want to violate. We could certainly do a value comparison here to hack around it, but I'd prefer not to.

Jamtis commented 9 years ago

I share your opinion. I like the in syntax. But it somehow disturbs me that es6-shim doesn't need Symbols to shim Maps but fails then they are broken. I think it's the propose of a shim library to provide the features as complete as possible.

ljharb commented 9 years ago

That's correct - but a shim should either provide a 100% reliant shim, or none at all. The existence of the well-known symbol properties on Object.prototype means the symbol shim is not a shim at all, it's what's been termed a "sham". es6-shim will work in any existing engine, or any shimmed-and-spec-compliant environment - but it's not designed or intended to work in a broken one. es6-symbol's shim leaves behind a broken and incomplete environment.

Jamtis commented 9 years ago

Alright I agree with your argument. I guess we can close this issue with a #wontfix.

ljharb commented 9 years ago

Agreed.

fwiw, changing name in object to name in object && typeof object[name] !== 'undefined' does seem to allow all tests to pass, and also prevents the issue with your scenario.

You may want to look into https://github.com/WebReflection/get-own-property-symbols which seems to be a much more reliable (ie, much closer to 100%) Symbol shim, that should work with es6-shim.

Jamtis commented 9 years ago

Btw thanks for the competent support.