Skip to content

refactor(NODE-4125): misc change stream improvements #3284

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

Merged
merged 10 commits into from
Jun 10, 2022

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Jun 3, 2022

What is changing?

As a part of NODE-4125, I made a couple of small refactors. NODE-4125 isn't done, but I broke these changes into a separate PR. I made 6 distinct changes:

  1. added another overload for .only describe blocks to our mocha config
  2. moved change stream cursor into its own file in the cursors subdirectory
  3. added a doc comment to the next<T> function, explaining what the blocking parameter does
  4. moved the tryNext function on the change stream to be grouped with the other iterator methods
  5. created a custom readable that extends Readable instead of using the Readable constructor method the Node library provides
  6. gets rid of a duplicate function that handled unresumable errors in favor of the existing class method

I think point 5 is probably the only change that will cause contention so I'm happy to revert if it we decide that's what we want to do.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Improved driver code.

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

@baileympearson baileympearson changed the title refactor(NODE-4125): misc change stream improvements refactor: misc change stream improvements Jun 3, 2022
@nbbeeken nbbeeken self-requested a review June 3, 2022 18:59
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 3, 2022
@baileympearson baileympearson force-pushed the NODE-4125-misc-change-stream-cleanups branch from bd8c314 to e79db9b Compare June 10, 2022 15:14
@baileympearson baileympearson requested a review from nbbeeken June 10, 2022 15:15
Adds our custom overloads of Mocha's functions to the ExclusiveSuiteFunction interface
in global.d.ts, so a describe.only doesn't cause everything to have type errors.
the _initialized and _needToClose variables were unnecessary.
- _initialized existed only to conditionally set _needToClose when the
stream was read from for the first time
- _needToClose is unnecessary, because cursor.close is a no-op
if the cursor is already closed.
@baileympearson baileympearson force-pushed the NODE-4125-misc-change-stream-cleanups branch from e79db9b to 7cdb01c Compare June 10, 2022 15:32
nbbeeken
nbbeeken previously approved these changes Jun 10, 2022
@nbbeeken nbbeeken changed the title refactor: misc change stream improvements refactor(NODE-4125): misc change stream improvements Jun 10, 2022
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 10, 2022
@dariakp dariakp requested a review from nbbeeken June 10, 2022 18:10
@nbbeeken nbbeeken merged commit 973d4ad into main Jun 10, 2022
@nbbeeken nbbeeken deleted the NODE-4125-misc-change-stream-cleanups branch June 10, 2022 20:41
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
Development

Successfully merging this pull request may close these issues.

2 participants