-
Notifications
You must be signed in to change notification settings - Fork 2k
promise(core): Add Promise Option to Mongoose #1560
Conversation
@@ -29,6 +29,8 @@ module.exports.connect = function (cb) { | |||
console.log(err); | |||
} else { | |||
|
|||
mongoose.Promise = config.db.promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just?
mongoose.Promise = global.Promise;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated that and since things have been moving to be as configurable as possible, I figured I'd follow that trend. I'm not opposed to changing it though.
You could in theory set it to be bluebird or q or any other promise library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't
db: { promise: global.Promise }
in config/env/default.js
enough ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sort of indifferent on whether or not to make this configurable. Personally, I like things to be as configurable as possible. However, in this case I can't think of any use case where having a different promise library to be used between, say, a dev env & test env would be useful.
If we do go with adding this into the environment configuration, I agree with @shanavas786 that having it in the default config is sufficient. This wouldn't stop someone from adding it to the others if they have a need to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Pending the outcome of the discussion on having this configurable.
@shanavas786 @mleanos @simison I consolidated to the default.js. |
@codydaig LGTM. Feel free to merge when you're ready. |
Fixes #1559