search

paulmillr / es6-shim

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

Use original map for data backing when possible. #429

Closed jdalton closed 7 years ago

jdalton commented 7 years ago

This PR attempts to address #422 by using the original map for data backing when possible to avoid linear lookups.

ljharb commented 7 years ago

Thanks!

What about browsers where -0 and 0 are not properly distinguished? There's also insertion order, browsers where iteration completion throws a StopIterator exception, browsers where "size" is a function instead of a getter, browsers where Map and Set aren't iterable except with an eval'd for-of, browsers that lack the facility to clear and create iterators for keys/values/entries and then combined with the fact that the shim itself mutates Map.prototype when it already exists - so you'd need to cache all of the existing prototype methods (like get, has, set, etc), or else you'd risk an infinite loop.

In other words, using the original Map is a great approach, but there are a fractal of issues caused by the very necessity of the shim in the first place :-/

jdalton commented 7 years ago

What about browsers where -0 and 0 are not properly treated?

I was hoping that was handled by the fast path. If not I'll rework to address it in the fast key path.

There's also insertion order, browsers where iteration completion throws a StopIterator exception,

Do you have a unit test to cover this? This is internal storage where the fast path has no concept of order since it's an empty object.

browsers where "size" is a function instead of a getter, browsers where Map and Set aren't iterable except with an eval'd for-of,

I'm not using size of the internal map.

browsers that lack the facility to clear and create iterators for keys/values/entries

This doesn't affect our internal map use.

the shim itself mutates Map.prototype when it already exists - so you'd need to cache all of the existing prototype methods (like get, has, set, etc), or else you'd risk an infinite loop.

Ya, I'll address that.

ljharb commented 7 years ago

I have a number of test cases that should cover it - you'd need to run the test HTML in a very large number of browsers (which sadly travis doesn't cover) to check it out.

Mind you I'm not saying all these things will definitely cause a problem - more that it's possible, and I want to both be confident in this PR's correctness and also avoid leaving behind a footgun for the future.

jdalton commented 7 years ago

Mind you I'm not saying all these things will definitely cause a problem - more that it's possible, and I want to both be confident in this PR's correctness and also avoid leaving behind a footgun for the future.

Cool. Ya, the use case I'm going for is to be a replacement for the linear search for object keys. That means it just needs to be a black box that is a yay or nay for if an object key is found. It can even go along with the array tracking as just a faster check but defer to the linear backing for when order is important.

jdalton commented 7 years ago

Looks like unit tests pass in IE11 and it resolves the perf issue :tada:

ljharb commented 7 years ago

Thanks, I'll give it a thorough review, and do some extensive testing in as many browsers as I can before merging this.

jdalton commented 7 years ago

Tested on Safari 7, 8, 9, 10 and IE 9, 10 with no Map related test fails.

ljharb commented 7 years ago

TC39 is this week so I'll need a bit more time to review; rest assured it's still on my list :-)

ljharb commented 7 years ago

Thanks!