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

Resolver construct to simplify promise/resolve/reject situations #1256

Closed
wants to merge 1 commit into from

Conversation

dvoytenko
Copy link
Contributor

No description provided.

/**
* @template T
*/
class Resolver {
Copy link
Member

Choose a reason for hiding this comment

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

can we rename this to Deferred as its close to the jquery Deferred object. (where its an instance that has a .promise property and .resolve/.reject methods). i dont feel strongly either way so feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's "Resolver" in Closure and YUL.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this should be Deferred, that's what all JS libraries call it.

@ampsauce
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: b18ab6e
Build details: https://travis-ci.org/ampsauce/amphtml/builds/99182606

(Please note that this is a fully automated comment.)

/** @const @type {!Promise<T>} */
this.promise = promise;
/** @const @type {function(T)} */
this.resolve = resolve.bind(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to bind here? I don't know of a promise implementation that changes behavior due the to this.

@jridgewell
Copy link
Contributor

Just note that Bluebird considers this an anti-pattern.

@cramforce
Copy link
Member

It's an anti pattern if overused. Promise's API not supporting this by
default is the right choice. But some cases are inherently better handled
with it. It is just important to never leak the deferred/resolver in a
public API and keep it constrained to a small scope.
Not a fan of Deferred. We are a bit "vorbelastet" because of
goog.async.Deferred :)
On Dec 29, 2015 8:32 AM, "Justin Ridgewell" [email protected]
wrote:

Just note that Bluebird considers this an anti-pattern
https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns#the-deferred-anti-pattern
.


Reply to this email directly or view it on GitHub
#1256 (comment).

@dvoytenko
Copy link
Contributor Author

The real antipattern is the leaking of resolver function itself. In some cases Resolver would make it better. That being said, I haven't yet bought into this idea either. I'll first take a look at how often we store the resolvers as instance vars. I did see recently an increase in those where we store pairs of promise/resolver. It's possible that those can be changed.

If I go ahead with this, I will put it into presubmit restriction set.

@dvoytenko
Copy link
Contributor Author

So, I looked through most of cases we have today, and my opinion is that in most of cases, it's possible to refactor code to avoid resolver leaker. So, I'm gonna close this PR.

@dvoytenko dvoytenko closed this Dec 30, 2015
@dvoytenko dvoytenko deleted the promise-res branch February 12, 2018 19:49
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.

5 participants