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

New Updated Code Breaks Promise Functions #27

Closed
devotox opened this issue Nov 11, 2014 · 7 comments
Closed

New Updated Code Breaks Promise Functions #27

devotox opened this issue Nov 11, 2014 · 7 comments

Comments

@devotox
Copy link

devotox commented Nov 11, 2014

This was meant to be a combination of both request and bluebird.

But now with the API changes you will no longer be able to use any of bluebird's methods other than .then and .catch.Especially one as useful as .bind(this).

Question: Will you be somehow proxying methods to Bluebird in an upcoming release or is this the way you will go from now on just adding prototyp methods directly to the request object?

@analog-nico
Copy link
Member

Thanks for your instant feedback!

Indeed, the Bluebird promise is hidden inside and 0.3.0 only exposes .then and .catch. To expose additional methods like .bind it only need 3 extra lines of code internally.

So which option do you prefer judging from how you use Request-Promise?

  1. Expose .bind as well
    • Pro: You don't need to change your existing code.
    • Contra: Since this will certainly be not the last method we need to expose as well, the request call object will get polluted more and more which increases the risk of naming conflicts. (Maybe the Request team decides to introduce a .bind method themselves...)
  2. Expose a .promise method that returns the Bluebird promise which is currently hidden internally
    • Pro: The request call object won't be polluted too much and you can instantly make use of the full Bluebird API
    • Contra: You need to change your existing code. (All rp(...).bind(...) calls convert to rp(...).promise().bind(...).)

@devotox
Copy link
Author

devotox commented Nov 11, 2014

I think the second way would be the best and most manageable and I would
happily update my code for it

On Nov 11, 2014 3:35 PM, "Nicolai Kamenzky" notifications@github.com
wrote:

Thanks for your instant feedback!

Indeed, the Bluebird promise is hidden inside and 0.3.0 only exposes .then
and .catch. To expose additional methods like .bind it only need 3 extra
lines of code internally.

So which option do you prefer judging from how you use Request-Promise?

  1. Expose .bind as well
    • Pro: You don't need to change your existing code.
    • Contra: Since this will certainly be not the last method we need
      to expose as well, the request call object will get polluted more and more
      which increases the risk of naming conflicts. (Maybe the Request team
      decides to introduce a .bind method themselves...)
      1. Expose a .promise method that returns the Bluebird promise which
        is currently hidden internally
    • Pro: The request call object won't be polluted too much and you
      can instantly make use of the full Bluebird API
    • Contra: You need to change your existing code. (All
      rp(...).bind(...) calls convert to rp(...).promise().bind(...).)


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

@analog-nico
Copy link
Member

Just to be sure: Only the first method in the chain needs to be .then or .catch. Further chaining can use all Bluebird methods.

rp(...).bind(...); // Currently not supported

rp(...).then(...).bind(...); // Already supported because .then() returns a full-fledged Bluebird promise

@devotox
Copy link
Author

devotox commented Nov 11, 2014

Yes this is true and technically you could use the .then and just pass the data through and then have a bluebird instance but the solution also aligned previously would be instantly robust with the .promise() function

@analog-nico
Copy link
Member

Indeed, thanks for your perspective. I will release 0.3.1 later today with .promise() included.

@devotox
Copy link
Author

devotox commented Nov 11, 2014

Excellent and thank you for a very useful library

@analog-nico
Copy link
Member

I just published version 0.3.1. Have fun with it. :)

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

No branches or pull requests

2 participants