Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

promise(core): Add Promise Option to Mongoose #1560

Merged
merged 6 commits into from
Oct 15, 2016
Merged

Conversation

codydaig
Copy link
Member

Fixes #1559

@@ -29,6 +29,8 @@ module.exports.connect = function (cb) {
console.log(err);
} else {

mongoose.Promise = config.db.promise;
Copy link
Member

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;

Copy link
Member Author

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.

Copy link
Contributor

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 ?

Copy link
Member

@mleanos mleanos Oct 13, 2016

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.

Copy link
Member

@mleanos mleanos left a 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.

@codydaig
Copy link
Member Author

@shanavas786 @mleanos @simison I consolidated to the default.js.

@mleanos
Copy link
Member

mleanos commented Oct 14, 2016

@codydaig LGTM. Feel free to merge when you're ready.

@codydaig codydaig merged commit 517dc32 into master Oct 15, 2016
@codydaig codydaig deleted the mongoosePromise branch October 15, 2016 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants