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!: Integrate MongoOptions parser into driver #2680

Merged
merged 7 commits into from
Jan 8, 2021

Conversation

nbbeeken
Copy link
Contributor

MongoOptions introduce a well typed HostAddress type that tracks
the correct shape of a host's address depending on its type.
The MongoClientOptions type inherit from node's TLS and Socket options
and use our allow list of options to maintain a connection between the
types and programmatic processing of options. The hierarchy of options
from MongoClient > Topology > Server > ConnectionPool > Connection have
been corrected and clarified for responsibilities.

NODE-2704

@nbbeeken nbbeeken force-pushed the NODE-2704/integrate-mongoOptions branch 7 times, most recently from 09174ae to 74232bc Compare December 22, 2020 17:57
@nbbeeken nbbeeken force-pushed the NODE-2704/integrate-mongoOptions branch from 74232bc to 687baea Compare January 2, 2021 05:19
@nbbeeken nbbeeken requested a review from emadum January 4, 2021 17:03
@nbbeeken nbbeeken marked this pull request as ready for review January 4, 2021 17:04
@nbbeeken nbbeeken force-pushed the NODE-2704/integrate-mongoOptions branch from 687baea to 13278cf Compare January 4, 2021 17:22
MongoOptions introduce a well typed HostAddress type that tracks
the correct shape of a host's address depending on its type.
The MongoClientOptions type inherit from node's TLS and Socket options
and use our allow list of options to maintain a connection between the
types and programmatic processing of options. The hierarchy of options
from MongoClient > Topology > Server > ConnectionPool > Connection have
been corrected and clarified for responsibilities.

NODE-2704
@nbbeeken nbbeeken force-pushed the NODE-2704/integrate-mongoOptions branch from 13278cf to 8f265c9 Compare January 4, 2021 18:14
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.

Very nice work here! 🥳 I had a few small questions/comments but overall I think this is looking great.

src/logger.ts Outdated
@@ -48,7 +48,7 @@ export class Logger {
this.className = className;

// Current logger
if (options.logger) {
if (!(options.logger instanceof Logger) && typeof options.logger === 'function') {
currentLogger = options.logger;
} else if (currentLogger == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: now that currentLogger defaults to console.warn above, I don't think this branch will ever trigger.

src/connection_string.ts Outdated Show resolved Hide resolved
test/unit/mongo_client_options.test.js Show resolved Hide resolved
src/sdam/topology.ts Outdated Show resolved Hide resolved
src/cmap/connect.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from emadum January 7, 2021 21:01
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 🚀

@nbbeeken nbbeeken merged commit b1bdb06 into master Jan 8, 2021
@nbbeeken nbbeeken deleted the NODE-2704/integrate-mongoOptions branch January 8, 2021 15:33
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.

2 participants