feat(NODE-7441): add ChangeStream.bufferedCount#4870
feat(NODE-7441): add ChangeStream.bufferedCount#4870typesafe wants to merge 4 commits intomongodb:mainfrom
ChangeStream.bufferedCount#4870Conversation
src/change_stream.ts
Outdated
| } | ||
|
|
||
| /** Returns the currently buffered documents length of the underlying cursor. */ | ||
| get bufferedCount(): number | undefined { |
There was a problem hiding this comment.
Let's change this to return number in all cases: if there is no cursor, we should return 0.
There was a problem hiding this comment.
Also, I think ideally the API should be a method instead of a getter so that AbstractCursor and ChangeStream implement the API in the same way to make them interchangeable
There was a problem hiding this comment.
Thanks @PavelSafronov & @nbbeeken, makes sense. I've aligned it with AbstractCursor.bufferedCount.
There was a problem hiding this comment.
Pull request overview
Adds a new bufferedCount() API to ChangeStream to expose how many documents are currently buffered by the underlying change stream cursor (NODE-7441).
Changes:
- Add
ChangeStream.bufferedCount()that delegates to the underlying cursor’sbufferedCount(). - Document the new method via a JSDoc comment.
Comments suppressed due to low confidence (1)
src/change_stream.ts:709
- The doc comment is a bit ungrammatical/unclear (“buffered documents length”). Consider rewording to something like “Returns the number of documents currently buffered by the underlying cursor.” so it’s consistent with other cursor docs.
/** Returns the currently buffered documents length of the underlying cursor. */
bufferedCount(): number {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| bufferedCount(): number { | ||
| return this.cursor?.bufferedCount() ?? 0; | ||
| } |
There was a problem hiding this comment.
This introduces a new public surface on ChangeStream. Please add test coverage (unit or integration) that validates it reports the underlying cursor’s buffered document count (and returns 0 when nothing is buffered), similar to the existing AbstractCursor bufferedCount tests.
There was a problem hiding this comment.
@typesafe Seems worth doing, ideally capturing your use case in some minor form would make sure the driver doesn't regress, I'm thinking specifically about your expectation of what tryNext does when bufferedCount is zero or non-zero.
This file is probably a good spot: test/integration/change-streams/change_stream.test.ts
Description
Summary of Changes
This addresses issue NODE-7441
Notes for Reviewers
N/A
What is the motivation for this change?
See JIRA
Release Highlight
ChangeStreams now have a
bufferedCount()method that matches cursorsIn some circumstances it may be desirable to determine if there are local documents stored in your change stream before invoking one of the async methods (
tryNext,hasNextetc.). ThechangeStream.bufferedCount()returns the number of documents remaining inside the change stream from the last batch.Shout out to @typesafe for contributing this feature!
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript