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-4788)!: use implementer Writable methods for GridFSBucketWriteStream #3808

Merged
merged 6 commits into from
Aug 15, 2023

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Aug 10, 2023

Description

What is changing?

  • Remove overrides of stream methods
  • Add overrides of _X stream methods that control the underlying implmentations
  • Remove manual event emitting
  • Ensure callback is always invoked asynchronously (nextTick)
Is there new documentation needed for these changes?
  • TBD

What is the motivation for this change?

Node.js Streams are supposed to be implemented with the _X methods, the write & end functions are maintained by Node.js, the _ methods define the implementation of a stream.

Release Highlight

Corrected GridFSBucketWriteStream's Writable method overrides and event emission

Our implementation of a writeable stream for GridFSBucketWriteStream mistakenly overrode the write() and end() methods, as well as, manually emitted 'close', 'drain', 'finish' events. Per Node.js documentation, these methods and events are intended for the Node.js stream implementation to provide, and an author of a stream implementation is supposed to override _write, _final, and allow Node.js to manage event emitting.

Since the API is still a Writable stream most usages will continue to work with no changes, the .write() and .end() methods are still available and take the same arguments. The breaking change relates to the improper manually emitted event listeners that are now handled by Node.js. The 'finish' and 'drain' events will no longer receive the GridFSFile document as an argument (this is the document inserted to the bucket's files collection after all chunks have been inserted). Instead, it will be available on the stream itself as a property: gridFSFile.

// If our event handler is declared as a `function` "this" is bound to the stream.
fs.createReadStream('./file.txt')
  .pipe(bucket.openUploadStream('file.txt'))
  .on('finish', function () {
    console.log(this.gridFSFile)
  })

// If our event handler is declared using big arrow notation,
// the property is accessible on a scoped variable
const uploadStream = bucket.openUploadStream('file.txt')
fs.createReadStream('./file.txt')
  .pipe(uploadStream)
  .on('finish', () => console.log(uploadStream.gridFSFile))

Since the class no longer emits its own events: static constants GridFSBucketWriteStream.ERROR, GridFSBucketWriteStream.FINISH, GridFSBucketWriteStream.CLOSE have been removed to avoid confusion about the source of the events and the arguments their listeners accept.

Fix manually emitted events from GridFSBucketReadStream

The GridFSBucketReadStream internals have also been corrected to no longer emit events that are handled by Node's stream logic. Since the class no longer emits its own events: static constants GridFSBucketReadStream.ERROR, GridFSBucketReadStream.DATA, GridFSBucketReadStream.CLOSE, and GridFSBucketReadStream.END have been removed to avoid confusion about the source of the events and the arguments their listeners accept.

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

@nbbeeken nbbeeken force-pushed the NODE-4788-stream-overrides branch from 0bde921 to e6aebb4 Compare August 10, 2023 19:53
@nbbeeken nbbeeken force-pushed the NODE-4788-stream-overrides branch from e6aebb4 to bb2fcfc Compare August 10, 2023 20:40
@nbbeeken nbbeeken changed the title fix(NODE-4788)!: correctly implement gridfs stream underscore methods fix(NODE-4788)!: use implementer Writable methods for GridFSBucketWriteStream Aug 11, 2023
@nbbeeken nbbeeken marked this pull request as ready for review August 11, 2023 17:21
src/gridfs/upload.ts Outdated Show resolved Hide resolved
src/gridfs/upload.ts Outdated Show resolved Hide resolved
src/gridfs/upload.ts Show resolved Hide resolved
test/integration/gridfs/gridfs_stream.test.js Outdated Show resolved Hide resolved
test/integration/gridfs/gridfs_stream.test.js Outdated Show resolved Hide resolved
@durran durran self-assigned this Aug 14, 2023
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 14, 2023
@nbbeeken nbbeeken requested a review from durran August 14, 2023 18:09
@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 15, 2023
@durran durran merged commit 7955610 into main Aug 15, 2023
@durran durran deleted the NODE-4788-stream-overrides branch August 15, 2023 19:16
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