feat: switch to native Promise #624
Conversation
|
@alexkvak This needs to be synced with master after old Node versions were dropped |
lib/channel_model.js
Outdated
| return self.closeBecause("Goodbye", defs.constants.REPLY_SUCCESS, | ||
| cb); | ||
| }); | ||
| // return new Promise(function(resolve, reject) { |
There was a problem hiding this comment.
Should this be commented out?
There was a problem hiding this comment.
I need some help with this.
If I replace Bluebird.fromCallback with commented code it seems that channel.accept is called after this promise resolved. So the behavior changes and I have no idea why. I'm sure you have more context with this.
| @@ -74,10 +98,16 @@ C.open = function() { | |||
|
|
|||
| C.close = function() { | |||
| var self = this; | |||
| return Promise.fromCallback(function(cb) { | |||
|
|
|||
| return Bluebird.fromCallback(function(cb) { | |||
There was a problem hiding this comment.
Am I misunderstanding something, or promisify could be used here instead of Bluebird?
There was a problem hiding this comment.
After dropping old Node version — it could
There was a problem hiding this comment.
Only Node 10+ is supported in master, so should be fine to use it now.
There was a problem hiding this comment.
btw .travis.yml in main still builds library for old Node.js versions
There was a problem hiding this comment.
Yes, I think it was just missed in 267dd4e. Probably Travis needs to be adjusted to only test for Node 10+ as well.
alexkvak
force-pushed
the
feat/switch-to-native-promise
branch
from
58d9bab to
1e59be1
Compare
lib/channel_model.js
Outdated
| return self.closeBecause("Goodbye", defs.constants.REPLY_SUCCESS, | ||
| cb); | ||
| }); | ||
| // return new Promise(function(resolve, reject) { |
There was a problem hiding this comment.
@squaremo could you help with those three places (lines 106, 203 and 243)
If I replace Bluebird.fromCallback with commented code it seems that channel.accept is called after this promise resolved. So the behavior changes and I have no idea why. I'm sure you have more context with this.
| self._rpc(defs.BasicConsume, fields, defs.BasicConsumeOk, cb); | ||
| }) | ||
| // return new Promise(function(resolve, reject) { |
There was a problem hiding this comment.
Should this be commented out?
| @@ -5,7 +5,6 @@ | |||
| 'use strict'; | |||
|
|
|||
| var defs = require('./defs'); | |||
| var Promise = require('bluebird'); | |||
| var inherits = require('util').inherits; | |||
There was a problem hiding this comment.
Unrelated to this PR but i just notices inherits, NodeJS docs mention discourage use of inherits and that class extends should be used instead.
|
Should also edit the types once this is ready https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/amqplib |
| @@ -1,15 +1,10 @@ | |||
| var raw_connect = require('./lib/connect').connect; | |||
| const { promisify } = require('util'); | |||
| const raw_connect = promisify(require('./lib/connect').connect); | |||
There was a problem hiding this comment.
promisify is undefined
|
I don't really recommend changing anything until tests are being migrated to jest and a proper memory leak detector is set up. There is a PR that fixes a memory leak, though either it isn't covered by tests or mocha's detector can't catch the case. Anyway, any change in the codebase may bring undesired behavior in the future, and it may take time to fix and release it. As I may see in the comments here, Bluebird's semantics differs a bit from Node's promise implementation. Also, @squaremo is quite a busy man and merges PRs if only 100% sure they are ok. I'm going to send a PR with jest migration this week. |
|
https://github.com/cloudamqp/amqp-client.js/ is a modern zero dependency AMQP library, implementing native promises, among other improvements. |
Thanks a lot! I'll take a look and return with feedback 👍 |
|
Use of native promises has been released in v0.10.0 via #689. This can be closed. |
No description provided.