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

fix(NODE-4186): accept ReadPreferenceLike in TransactionOptions type #3425

Merged
merged 2 commits into from
Oct 3, 2022

Conversation

noahsilas
Copy link
Contributor

Description

This commit expands the TransactionOptions type definition to allow values like 'primary'.

What is changing?

Before, the type definition only allowed passing an instance of the ReadPreference class. This differs from documented and common usage. We expand the type definition from ReadPreference to ReadPreferenceLike.

Updating this type annotation will reduce confusion for Typescipt users of the library.

Is there new documentation needed for these changes?

No, this makes the types match the existing documentation more closely. For instance, in https://www.mongodb.com/docs/drivers/node/current/fundamentals/transactions/#transaction-settings, this API is documented as accepting a string:

const transactionOptions = {
readPreference: 'primary',
readConcern: { level: 'local' },
writeConcern: { w: 'majority' },
maxCommitTimeMS: 1000
};
session.startTransaction(transactionOptions);

What is the motivation for this change?

The documented code snippet above should not cause a Typescript error.

We can also see in https://github.com/mongodb/node-mongodb-native/blob/48fa588362/src/transactions.ts#L108 that the transaction constructor will pass its options through to ReadPreference.fromOptions, which will handle coercing a string into a ReadPreference instance, so the documentation appears to be correct while the type definition is lacking.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

This API is documented as accepting a string: from
https://www.mongodb.com/docs/drivers/node/current/fundamentals/transactions/#transaction-settings:

> const transactionOptions = {
>   readPreference: 'primary',
>   readConcern: { level: 'local' },
>   writeConcern: { w: 'majority' },
>   maxCommitTimeMS: 1000
> };
> session.startTransaction(transactionOptions);

We further see in https://github.com/mongodb/node-mongodb-native/blob/48fa588362/src/transactions.ts#L108
that the transaction constructor will pass its options through to
`ReadPreference.fromOptions`, which will handle coercing a string into a
ReadPreference instance.

Updating this type annotation will reduce confusion for Typescipt users
of the library.
@bajanam bajanam added the External Submission PR submitted from outside the team label Oct 3, 2022
@nbbeeken nbbeeken added the Team Review Needs review from team label Oct 3, 2022
@nbbeeken
Copy link
Contributor

nbbeeken commented Oct 3, 2022

@noahsilas Thanks for the help here! I've added a test to just double check that this keeps working at runtime (as it already has been)

@nbbeeken nbbeeken changed the title fix(NODE-4186): Accept ReadPreferenceLike in transaction options fix(NODE-4186): accept ReadPreferenceLike in TransactionOptions type Oct 3, 2022
@baileympearson baileympearson merged commit dc62bcb into mongodb:main Oct 3, 2022
@noahsilas
Copy link
Contributor Author

Thanks for the support on the testing @nbbeeken ! Appreciate it a lot 😄

ZLY201 pushed a commit to ZLY201/node-mongodb-native that referenced this pull request Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Submission PR submitted from outside the team Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants