Update Promise implementation to match latest ES6 spec. #345
Conversation
| @@ -285,6 +292,11 @@ | |||
| return typeof x === 'function' && _toString(x) === '[object Function]'; | |||
| }, | |||
|
|
|||
| IsConstructor: function (x) { | |||
| // We can't tell callables from constructors in ES5 | |||
| return ES.IsCallable(x); | |||
There was a problem hiding this comment.
we could do a "is not arrow function" check here too if we want, but that may not help much
There was a problem hiding this comment.
Yeah, my motivation for breaking it out is just (a) to mirror the spec more closely so you can read some of our implementations and match them step-by-step to the spec, and (b) to provide a central place to put special cases for IsConstructor (like arrow functions, or whatever) when we find a good reason to do so (for compatibility in some edge case). But it doesn't seem worth while to start cluttering up IsConstructor yet until we have the use case for it.
There was a problem hiding this comment.
Agreed, this is strictly an improvement as-is
|
Thanks for doing this! I think the "default species" stuff, and the "Array.from" stuff, might work better as separate PRs first, to keep this one smaller. |
Includes the erratum for Promise.resolve discussed in paulmillr#344. Also fixes the Map/Set constructors, since the spec uses [[Construct]] now instead of a hidden create method, and the previous version misapplied `Symbol.species` in this role. Also added ES.IteratorClose and updated a number of places where that is used in the latest spec, including in the implementation of `Array.from`. Closes: paulmillr#239 Closes: paulmillr#344
|
Splitting the patch would require me to fixup the old implementation of Promises, just before I got rid of it. I'd prefer not to waste the effort, assuming you can manage reviewing this as is. |
|
Hm, it seems like you could leave the |
|
(also it looks like the tests are failing because we're not clobbering the native |
There are a number of different bugs in v8's Promise implementation, starting with node 0.12 or so. Promise.all() doesn't accept array-likes, Promise.race() sometimes doesn't reject properly, Promise.then() invokes thenables incorrectly, etc. Many of these are hard to test precisely because you need to examine an asynchronous result. So instead use correct Promise.resolve() implementation as a canary to indicate a mature Promise implementation, and shim out implementations which don't get Promise.resolve() right.
|
Updated the patch to fix the wrapping-paren issue, and added a test on Promise.resolve() which should correct the test failures. |
| @@ -20,7 +29,7 @@ describe('Evil promises should not be able to break invariants', function () { | |||
| }; | |||
|
|
|||
| var calledAlready = false; | |||
| Promise.resolve(evilPromise).then(function (value) { | |||
| new Promise(function (r) { r(evilPromise); }).then(function (value) { | |||
There was a problem hiding this comment.
Hmm - Promise.resolve should have the same behavior as using resolve from new Promise, shouldn't it?
There was a problem hiding this comment.
Promise.resolve has this sorta-sketchy "is the argument of the same class as this" shortcut, which is what the new Promise version is explicitly avoiding.
|
Thanks for bearing with me - this looks awesome, much appreciated |
Update Promise implementation to match latest ES6 spec.
Includes the erratum for Promise.resolve discussed in #344.
Also fixes the Map/Set constructors, since the spec uses [[Construct]] now instead of a hidden create method, and the previous version misapplied
Symbol.speciesin this role.Also added ES.IteratorClose and updated a number of places where that is used in the latest spec, including in the implementation of
Array.from.Fixes issue #239 and #344.