Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support to protocol to be 'monogodb+srv' #458

Merged
merged 2 commits into from
Sep 12, 2018

Conversation

raymondfeng
Copy link
Contributor

See https://docs.mongodb.com/manual/reference/connection-string/

Description

We don't have a way to specify mongodb+srv://. This PR makes it configurable.

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nitpick. LGTM otherwise.

lib/mongodb.js Outdated
@@ -49,13 +49,17 @@ exports.generateMongoDBURL = generateMongoDBURL;
* Generate the mongodb URL from the options
*/
function generateMongoDBURL(options) {
// See https://docs.mongodb.com/manual/reference/connection-string/#dns-seedlist-connection-format
// It can be `mongodb+srv` now.
var protocol = options.protocol || 'mongodb';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency can we make this options.protocol = options.protocol || 'mongodb'

@raymondfeng raymondfeng force-pushed the allow-mongodb-srv-protocol branch from cf2bde0 to 695bb20 Compare September 12, 2018 17:46
Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for the memwatch change.

"eslint": "^5.1.0",
"eslint-config-loopback": "^10.0.0",
"loopback-datasource-juggler": "^3.0.0",
"memwatch-next": "^0.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay :)

Makefile Outdated
@@ -19,6 +19,7 @@ b benchmark benchmarks:

.PHONY: l ld leak leak-detection
l ld leak leak-detection:
npm i memwatch-next --no-save
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use @airbnb/node-memwatch instead here. It's compatible with Node 8/10. And then can you please update the Leak Detection section of the readme explaining that it only works on Node 8/10.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants