Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: Add MongoOptions interface #2616

Merged
merged 6 commits into from
Nov 19, 2020
Merged

feat: Add MongoOptions interface #2616

merged 6 commits into from
Nov 19, 2020

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Nov 9, 2020

Adds MongoOptions representing the internal view of the specified options.

NODE-2698

A number of options are seemingly omitted but instead are brought in by extending NodeJS's connection options. Particularly with tls options we have a number of translations from ssl to tls options and then another set of translations to NodeJS's equivalent definition. I've left documentation of the equivalent names and they will be resolved by the following work.

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM, with one small readability nit

src/mongo_client_options.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from mbroadst November 13, 2020 20:23
@nbbeeken nbbeeken force-pushed the NODE-2698/MongoOptions branch 2 times, most recently from ba4fe1f to e61706c Compare November 18, 2020 17:31
@nbbeeken nbbeeken force-pushed the NODE-2698/MongoOptions branch from e61706c to 956b500 Compare November 19, 2020 17:44
Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

LGTM, left a single thought but I don't feel strong about it at all

/**
* Turn these options into a reusable connection URI
*/
toURI(): string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
toURI(): string;
toConnectionString(): string;

just a thought? I'm the one that introduced calling it a URI, but I think the consensus across the drivers and the company is calling it a connection string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Connection String URI Format
It's one of those both ways is right, I'll leave it as is, and we can consider renaming it when we get to the ticket that implements it since there might be some questions about how we want to expose it. These options are supposed to be internal, kind of, so is it something we'd want people to use: client.options.toURI() or would we surface something on the client itself.

@nbbeeken nbbeeken merged commit 54c456b into master Nov 19, 2020
@nbbeeken nbbeeken deleted the NODE-2698/MongoOptions branch November 19, 2020 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants