Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Promise polyfill #520

Closed
wants to merge 3 commits into from
Closed

Add Promise polyfill #520

wants to merge 3 commits into from

Conversation

arv
Copy link
Collaborator

@arv arv commented Dec 7, 2013

This adds a port of Andreas Rossbergs Promise implementation which is what V8 is based on too.

It uses Yehuda's async/asap function to do the async call. This is done by adding a new npm dependency. Fortunately they are using ES6 modules so it is a simple import.

Since Promises are always async I had to add support for async tests to the feature tests. These are done by a new comment at the top, just like we have done before with other comments.

This new runtime file now requires compilation

@ghost ghost assigned johnjbarton Dec 7, 2013
@arv
Copy link
Collaborator Author

arv commented Dec 7, 2013

@johnjbarton

@slightlyoff Promise added for you.

@stefanpenner
Copy link

last i checked the v8 implementation was lagging behind the spec. We should likely wait till it is improved (or improved) or look to another implementation.

A faulty promise implementation will cause nothing but pain and agony, if we can prevent that that would be the best.

@arv
Copy link
Collaborator Author

arv commented Dec 8, 2013

The main reason for doing this is to get rid of Deferred in favor of Promise. These are already compatible since both are 'thenable'. I have a followup patch that does this.

I could remove everything but the constructor and then if that would make you feel happier?

@stefanpenner
Copy link

@arv no don't doit to make me happy, I was merely "bringing a warning", do with it as you please.

My recommendation would be to grab whichever one suites your needs from the list of compliant implementations: http://promisesaplus.com/implementations

@arv
Copy link
Collaborator Author

arv commented Dec 8, 2013

@stefanpenner It is a good point that V8's Promise is not A+ compatible at this point.

I think this is better than the Deferred we have so I still want to commit this. I'd be happy to work on fixing V8's promise as needed.

@arv
Copy link
Collaborator Author

arv commented Dec 8, 2013

I updated the Promise implementation. It now passes almost all the Promises/A+ tests. It fails some Promise.all test because of the implementation does not use an iterator at this point.

@stefanpenner
Copy link

@arv awesome!

}

// This should really be a WeakMap.
var thenableSymbol = '@@thenable';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'This' confuses me, because I don't see a string constant as an obvious but inferior alternative to WeakMap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to depend on weak maps just yet so instead we use an expando.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the weak map stuff might get dropped for now.

(cc @domenic)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I killed it recently; sorry about the churn :(. domenic/promises-unwrapping@65a5e62 See also domenic/promises-unwrapping#79 where a possible replacement is being discussed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I'll update.

@johnjbarton
Copy link
Contributor

lgtm, two questions.

@arv arv closed this in 08a8188 Dec 9, 2013
@arv arv deleted the promise branch December 9, 2013 03:52
function promiseCoerce(constructor, x) {
if (isPromise(x)) {
return x;
} else if (x && 'then' in Object(x)) { // can't test for callable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is out of context, so I might not be understanding what your comment means, but you can definitely test for callability: typeof then === "function".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionness != callability. But it'll have to do.
On 9 Dec 2013 08:10, "Domenic Denicola" notifications@github.com wrote:

In src/runtime/polyfills/Promise.js:

  •    chain(y, deferred.resolve, deferred.reject);
    
  •  else
    
  •    deferred.resolve(y);
    
  • } catch (e) {
  •  deferred.reject(e);
    
  • }
  • });
    +}

+// This should really be a WeakMap.
+var thenableSymbol = '@@thenable';
+
+function promiseCoerce(constructor, x) {

  • if (isPromise(x)) {
  • return x;
  • } else if (x && 'then' in Object(x)) { // can't test for callable

This is out of context, so I might not be understanding what your comment
means, but you can definitely test for callability: typeof then ===
"function".


Reply to this email directly or view it on GitHubhttps://github.com//pull/520/files#r8201363
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually is, if you look at the definition of typeof and IsCallable in the spec. Both are true if and only if the object in question has a [[Call]] internal method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I based this on https://github.com/rossberg-chromium/js-promise/blob/master/promise.js and fixed some obvious bugs. I guess I missed this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants