paulmillr / es6-shim

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

`Promise` is broken and not overridden in Chrome Canary #372

Closed ljharb closed 8 years ago

ljharb commented 8 years ago

There are currently 10 test failures: dealing with robustness against tampering, evil promises breaking invariants, incorrect subclassing, and calling thenables the right number of times and in the right tick.

These will only need to be detected and fixed if they make it to Chrome Stable, but it might be worth getting ahead of them and fixing them now.

Also see https://rawgit.com/paulmillr/es6-shim/master/test/native.html?grep=Promise*|promise*

caitp commented 8 years ago

I've got a CL with a fix for some of the listed failures, and it may involve removing the non-standard APIs too, it depends if we can get away with that (eg, if it's used by Chrome extensions or node modules, may have to hold off on that --- and on landing the fix, it seems :(). On the other hand, it might hurt perf for the time being too.

Most of the @@species stuff is in @littledan's hands atm, which may be part of the "subclassing" bugs you're seeing

ljharb commented 8 years ago

Awesome, thanks - my main concern is patching whatever makes it into stable. If it'll all be fixed by that time, then I don't have to patch Chrome forever, so I'm happy!

littledan commented 8 years ago

You definitely shouldn't have to patch it forever, but we also definitely aren't spec-compliant at the moment. @@species is at the top of my todo list after landing my current patches out for review and preparing for the upcoming TC39 meeting.

ljharb commented 8 years ago

@littledan Glad to hear it! If you could help by giving me some advance warning before any remaining violations are going to hit stable, then I'll wait til then to patch them.

zloirock commented 8 years ago

@littledan V8 Promise implementation has much more serious problems than @@species.

caitp commented 8 years ago

@zloirock see comment 1, these are getting fixed, although might be a while :l

littledan commented 8 years ago

@ljharb I don't things are going to get any worse, only better. Have you been seeing them getting worse?

ljharb commented 8 years ago

@littledan worse only in the sense that Chrome Canary has now fixed whichever Promise bug causes es6-shim to clobber it with its own implementation in Chrome stable, but not yet fixed other Promise bugs. Thus, I'd have to add further runtime detections to patch Canary's Promise.

caitp commented 8 years ago

if we can get it passing test262 and promise/aplus tests, it should be alright, no?

littledan commented 8 years ago

@ljharb Do you think this will break the web, if your users won't be able to update? We could instead roll that patch back and deliver all the Promise fixes as a package, if that's what's needed for web compatibility.

ljharb commented 8 years ago

@caitp theroetically yes - although i'm quite confident test262 has gaps that es6-shim's tests cover :-) i can't run test262 in a browser yet, or else I'd PR in the differences.

@littledan I don't know if anybody's already relying on es6-shim's spec-compliant semantics, but I'd assume so. I don't actually want "all or nothing" fixes - I'm fine with iterative patches - I'd just like to write as few will-live-forever-runtime-detections as possible :-)

caitp commented 8 years ago

i can't run test262 in a browser yet, or else I'd PR in the differences.

ping @bterlson (and whoever else) about updating test262.ecmascript.org to include newer tests ;)

there is also https://github.com/bterlson/test262-harness with support for D8 and jsshell, although there may be bugs. Other test harnesses in v8 repo, and probably mozilla-central too (nevermind, the version of test262 included in mozilla-central is ancient history).

bterlson commented 8 years ago

test262-harness works great with d8 (I run this occasionally). Getting a d8 build is an exercise left to the reader though (unlike jsshell, there don't appear to be binaries available).

Yes, test262.ecmascript.org needs updated. Anyone have bandwidth? :)