paulmillr / es6-shim

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

Add missing checks to Promise.resolve and Promise.reject. #379

Closed cscott closed 8 years ago

cscott commented 8 years ago

These checks were overlooked when @@species was resolved from these methods.

Discovered by running the ES2015 test262 test suite against es6-shim.

ljharb commented 8 years ago

Ah, thanks - could we add test coverage for these to this repo also?

ljharb commented 8 years ago

These checks are already covered by IsConstructor inside PromiseCapability - I added https://github.com/paulmillr/es6-shim/commit/3d5bc93597ce89eb05630bcbe194cbf8e8ce3383 to be sure.

Can you share the test262 test conditions that cause a failure without this PR's changes?

cscott commented 8 years ago

It's the built-ins/Promise/resolve/context-non-object-with-promise.js test case which is failing. It passes undefined as the context object, but a valid Promise as the argument, so it never gets to the check in IsConstructor. I added PR #381 to demonstrate how test262 test cases could be run on es6-shim, which should let you reproduce the failure.

ljharb commented 8 years ago

The only difference I see in the test262 tests is that they're voiding the constructor property on the promise they pass in - however, looking at es6-shim's code path for resolve/reject, they both immediately do effectively new PromiseCapability(this), which in its first line throws if IsConstructor fails for the argument, which it would for any non-function "this" value.

I definitely see the test failing in #381 but I'm mystified as to why.

ljharb commented 8 years ago

aha, although Promise.reject already works fine, Promise.resolve has an ES.IsPromise check inside it.

ljharb commented 8 years ago

@cscott Why does resolve have that check but not reject?

ljharb commented 8 years ago

I'm pursuing that question with the committee members, but in the meantime, if you could freshly rebase this on top of latest master I'd love to merge it ASAP. Thanks!

cscott commented 8 years ago

Rebased.

ljharb commented 8 years ago

Thanks!