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

Exposes some promise methods #28

Merged
merged 3 commits into from
Nov 17, 2014
Merged

Conversation

hjpbarcelos
Copy link
Contributor

Exposes finally and the compatibility methods caught and lastly.

Henrique J. P. Barcelos added 3 commits November 13, 2014 17:01
Exposes `finally` and the compatibility methods `caught` and `lastly`.
@analog-nico
Copy link
Member

Let's discuss this on Gitter.

@analog-nico
Copy link
Member

I am a bit hesitant to expose more and more methods of the Bluebird promise. IMHO this pollutes the original Request object and risks name collisions in the future. (E.g. the Request team might decide to introduce a finally method themselves.) That is why we decided to introduce .promise(). (See issue #27)

However, on the other side I understand that it is confusing for the user that only .then(...) and .catch(...) are available as the first method in a promise chain. That is why I exposed .catch(...) right away with version 0.3.0. I think rp.post(...).catch(...) is a common use case.

Would be rp(...).finally() be a common use case as well? rp(...).then(...).finally() makes sense to me but rp(...).finally() would be some fire and forget request.

What is your opinion?

@hjpbarcelos
Copy link
Contributor Author

About the caught()method, is just a compatibility issue for older ECMAScript versions. Since we all agree in exposing catch, IMO, caught should be exposed too.
About the .finally() method, I agree with you @analog-nico, it's not an usual use case, but would come in hand when you just need to forward a request response, independently of its result.

analog-nico added a commit that referenced this pull request Nov 17, 2014
Exposes some promise methods
@analog-nico analog-nico merged commit 914a7be into request:master Nov 17, 2014
@analog-nico
Copy link
Member

I just published version 0.3.2 to npm which includes .finally(...). As discussed on Gitter I chose to leave .caught(...) and .lately(...) out in order to keep the Request namespace as clean as possible.

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.

2 participants