♻️ Use mongodb promises
#140
Conversation
[`mongodb@5`][1] removes support for using callbacks, forcing consumers to use promises instead. This non-breaking change does the minimum amount of work to move us to using `mongodb` promises, while keeping all our interfaces intact. [1]: https://github.com/mongodb/node-mongodb-native/releases/tag/v5.0.0
alecgibson
force-pushed
the
internal-promises
branch
from
aa03087 to
21a5ce5
Compare
index.js
Outdated
| this.pendingConnect = []; | ||
| this._connect(mongo, options); | ||
| var connection = this._connect(mongo, options); | ||
| this.mongo = connection.then(function(result) { |
There was a problem hiding this comment.
This seems to be changing from a DB instance to a Promise<DB>. Since this isn't an underscore-prefixed property, I'd like to avoid this being a breaking change if possible. The published driver defers populating mongo anyways. The promise versions can go onto new (internal?) variables if needed.
Same for mongoPoll.
The underscore-prefixed properties are fine to change to promises, though we should consider renaming them.
There was a problem hiding this comment.
On that note, this.pendingConnect wasn't underscored; does that make this breaking anyway if I remove that...?
index.js
Outdated
| }; | ||
| if (typeof mongo === 'function') { | ||
| return new Promise(function(resolve, reject) { | ||
| mongo(function(error, db) { |
There was a problem hiding this comment.
The mongo function can return either a MongoClient or DB, so let's name it to reflect that.
Note - It also could be possible for a user's mongo function to return a DB even if they're on a recent Mongo driver version. It'd be a breaking change if we required it to be a MongoClient always.
There was a problem hiding this comment.
Are you sure about that? The existing code looks like it assumes it's always a MongoClient (ie it can call .db() on the result)
index.js
Outdated
| .then(function() { | ||
| callback(null); | ||
| }) | ||
| .catch(callback); |
There was a problem hiding this comment.
I remembered why I prefer using the 2-arg form of then(), from discussion last week.
It's specifically when doing interop back to callbacks. If the callback(null) throws, then the catch here will invoke the callback a second time.
Using the 2-arg form of then does not result in that issue:
.then(function() {
callback(null);
}, callback)We should also switch to 2-arg form of then in the other places where relevant too.
There was a problem hiding this comment.
Ew. But fine.
For the record, I've also had a quick test with chained promises in this style (eg promise.then(cb1, cb).then(cb2, cb)), because I was worried that catching an early rejection in a chain would rescue and continue chain execution, but it doesn't work like that so it seems okay.
alecgibson
force-pushed
the
internal-promises
branch
from
91fab6b to
f260786
Compare
- Turn `mongo` and `mongoPoll` back into `Db` instances instead of `Promise<Db>` - Use 2-argument form of `.then()` to avoid callbacks being invoked twice if the callback itself throws
alecgibson
force-pushed
the
internal-promises
branch
from
f260786 to
9116e04
Compare
There was a problem hiding this comment.
Change LGTM, assuming we can get passing tests.
One new assertion from here seems to be failing
https://github.com/share/sharedb/pull/590/files#diff-ecbf5ca870ee70ce75c048bebe7b69654963ac89d372212970b196e323c9b8f1R190
mongodb@5removes support for using callbacks, forcing consumers to use promises instead.This non-breaking change does the minimum amount of work to move us to using
mongodbpromises, while keeping all our interfaces intact.