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: remove top-level write concern options #2642

Merged
merged 10 commits into from
Dec 16, 2020

Conversation

HanaPearlman
Copy link
Contributor

@HanaPearlman HanaPearlman commented Nov 30, 2020

Description

Code changes include: the WriteConcern type now has only one property: writeConcern: WriteConcern | WriteConcernSettings, where WriteConcernSettings (naming up for debate) encompasses the old top-level options. WriteConcern.fromOptions ignores the old top-level WC options.

The change to legalOptionNames in db.ts means the old top-level WC options will be filtered out when creating a Db. The change to validOptionNames in operations/connect.ts means it will either error or warn (configurable) if the old top-level WC options are passed to a MongoClient.

Test changes include everything from #2624 and now all options that are not being used to create a connection string use the writeConcern key.

NODE-1722

@HanaPearlman HanaPearlman changed the title Node 1722/master/deprecate wc options feat: remove top-level write concern options Nov 30, 2020
@nbbeeken nbbeeken self-assigned this Nov 30, 2020
@nbbeeken nbbeeken force-pushed the NODE-1722/master/deprecate-wc-options branch from ed0868f to b4346bb Compare November 30, 2020 17:17
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, mod one nit request 👍

}

if (connStrOptions.srvHost) {
connStrOpts.srvHost = connStrOptions.srvHost;
}

// Any write concern options from the URL will be top-level, so we manually
// move them options under `object.writeConcern`
const wcKeys = ['w', 'wtimeout', 'j', 'journal', 'fsync'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to see this in a const exported by write_concern.ts, to make future maintenance easier. Perhaps we can restore the export of writeConcernKeys that was deleted and add the missing journal key in?

@nbbeeken nbbeeken force-pushed the NODE-1722/master/deprecate-wc-options branch 2 times, most recently from 5d2b9aa to 7e543c4 Compare December 7, 2020 18:55
@nbbeeken nbbeeken force-pushed the NODE-1722/master/deprecate-wc-options branch from 7e543c4 to e1246db Compare December 15, 2020 16:45
@nbbeeken nbbeeken merged commit 6914e87 into master Dec 16, 2020
@nbbeeken nbbeeken deleted the NODE-1722/master/deprecate-wc-options branch December 16, 2020 16:34
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