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-5213): ChangeStream.tryNext() should return TChange type #3649

Merged
merged 4 commits into from Apr 19, 2023

Conversation

alecgibson
Copy link
Contributor

Description

What is changing?

Update ChangeStream.tryNext() return type to be consistent with ChangeStream.next().

The underlying AbstractCursor has the same return type for both.

Is there new documentation needed for these changes?

If these API docs aren't auto-generated, then yes.

What is the motivation for this change?

Avoiding having to cast when using ChangeStream.tryNext().

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
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@alecgibson
Copy link
Contributor Author

I didn't know if tests were needed for type changes; I'm assuming making a type stricter, and it still building is good enough...?

@nbbeeken
Copy link
Contributor

nbbeeken commented Apr 19, 2023

Thank you for the improvement @alecgibson! Changing TS types to something more strict is always a little gray but reflecting the TChange return type is certainly more correct and useful, so we're happy to pull it in for the next release. I just threw in some quick tests to make sure our three methods for receiving changes stay the same.

Edit: The PR title changes are so we can call attention to it in the release notes

@nbbeeken nbbeeken changed the title chore(NODE-5213): fix ChangeStream.tryNext() return type fix(NODE-5213): fix ChangeStream.tryNext() return type Apr 19, 2023
@nbbeeken nbbeeken changed the title fix(NODE-5213): fix ChangeStream.tryNext() return type fix(NODE-5213): ChangeStream.tryNext() should return TChange type Apr 19, 2023
@nbbeeken nbbeeken added the Team Review Needs review from team label Apr 19, 2023
@nbbeeken nbbeeken self-assigned this Apr 19, 2023
nbbeeken
nbbeeken previously approved these changes Apr 19, 2023
@nbbeeken nbbeeken merged commit 3b58ca1 into mongodb:main Apr 19, 2023
16 of 21 checks passed
@alecgibson alecgibson deleted the NODE-5213 branch April 19, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants