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

Migrate when -> bluebird #295

Merged
merged 4 commits into from
Nov 1, 2016
Merged

Conversation

nfantone
Copy link
Contributor

@nfantone nfantone commented Nov 1, 2016

This is a new take on #158. I realize, since that PR has been seating there for a year and a half now, it won't ever be updated.

I won't add much to what was already commented on #158, except that this time around everything seems to be working fine, including unit tests. No API changes, AFAIK. This should be completely transparent to users. It also gets rid of the so-called "deferred anti-pattern", widely used in the library.

@michaelklishin
Copy link

You can verify that rabbitmq-tutorials are still functional with this change just to gather another data point :)

@nfantone
Copy link
Contributor Author

nfantone commented Nov 1, 2016

@michaelklishin Good idea. Also, you made me realize that I didn't remove when from the examples on this repo. I'll add some more commits in a minute.

@nfantone
Copy link
Contributor Author

nfantone commented Nov 1, 2016

@michaelklishin Examples on rabbitmq-tutorials seem to be the same as the ones included in this repo. So, they should work just fine.

@nfantone
Copy link
Contributor Author

nfantone commented Nov 1, 2016

@squaremo Tests running on node 4 threw a TIMEOUT error while trying to connect to the broker. Every other job in the matrix passed just fine.

screen shot 2016-11-01 at 12 39 07 pm

If you retry the build, it should be good to go.

Also, you should really consider:

@nfantone
Copy link
Contributor Author

nfantone commented Nov 1, 2016

@squaremo Also, if you worry about users that may have been using when specific, non-standard, Promise methods, you should know that all of its API is defined in bluebird one way or another. Conversion is trivial.

Here's a quick "migration guide" from when -> Bluebird:

when.done(handleResult, handleError) -> .done
when.then(onFulfilled) -> .then
when.spread(onFulfilledArray) -> .spread
when.fold(combine, promise2) -> .join
when.catch(onRejected) -> .catch
when.finally(cleanup) -> .finally
when.yield(x) -> .return
when.else(x) -> .catchReturn
when.tap(onFulfilledSideEffect) -> .tap
when.delay(milliseconds) -> .delay
when.timeout(milliseconds, reason) -> .timeout
when.inspect() -> .isRejected / .isFulfilled / .isPending / .isCancelled
when.with(thisArg) -> .bind
when.progress(onProgress) (deprecated API)

@squaremo squaremo merged commit 23b2447 into amqp-node:master Nov 1, 2016
@squaremo
Copy link
Collaborator

squaremo commented Nov 1, 2016

I bit the bullet 🎉 I think this warrants a 0.5 release (a minor version bump v1 right?).

Many thanks @nfantone, and to @myndzi for the first time around. And to everyone for being patient, or at least quiet :)

@nfantone nfantone deleted the enhancement/bluebird branch November 1, 2016 21:49
@nfantone
Copy link
Contributor Author

nfantone commented Nov 1, 2016

@squaremo Yey! 0.5.0 seems fine to me, as changes are backwards compatible with minor tweaks. And those changes should only apply if you have been using whenspecific extensions.

@wtgtybhertgeghgtwtg
Copy link

It's a little late to say something, but wouldn't it be a better time to just the minimum required node version to 0.12 and use native promises, since support for node 0.10 ended today?

@nfantone
Copy link
Contributor Author

nfantone commented Nov 2, 2016

  1. @wtgtybhertgeghgtwtg, sir, you've awarded most unpronounceable name in all Github kingdom.
  2. bluebird >>> native Promise. My opinion (and also others' - see this, this and this here).

@wtgtybhertgeghgtwtg
Copy link

It's a dependency that duplicates native functionality for negligible and dwindling performance benefits.

@nfantone
Copy link
Contributor Author

nfantone commented Nov 2, 2016

No, it's not.

But you are certainly free to open a new PR with your proposed changes.

@wtgtybhertgeghgtwtg
Copy link

If such a PR would be accepted, I would be happy to do so.

@squaremo squaremo mentioned this pull request Feb 8, 2017
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.

4 participants