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

[pg-stream] Async iterable #2050

Closed
wants to merge 1 commit into from
Closed

[pg-stream] Async iterable #2050

wants to merge 1 commit into from

Conversation

matthieusieben
Copy link
Contributor

  • Does not emit error events
  • Properly destroy the stream in case of error while reading
  • Does not emit close to aclively close. close avents should be used to notify that the stream was closed, and not that the stream must be closed.
  • No need to keep a custom this.batchSize. The internal Readable machinery relies on a highWaterMark that does exactly that when reading more rows.
  • No need to make an additional cursor.read() when we received fewer rows than requested (the cursor will always return exactly the requested number of rows, unless there are no more rows to be read from the query).

- Does not emit `error` [events](https://nodejs.org/api/stream.html#stream_errors_while_reading)
- Properly destroy the stream in case of error while reading
- Does not emit `close` to aclively close. `close` avents should be used to notify that the stream *was* closed, and not that the stream must be closed.
- No need to keep a custom `this.batchSize`. The internal Readable machinery relies on a `highWaterMark` that does exactly that when reading more rows.
- No need to make an additional `cursor.read()` when we received fewer rows than requested (the cursor will always return exactly the requested number of rows, unless there are no more rows to be read from the query).
brianc added a commit that referenced this pull request Dec 28, 2019
There were some subtle behaviors with the stream being implemented incorrectly & not working as expected with async iteration.  I've modified the code based on #2050 and comments in #2035 to have better test coverage of async iterables and update the internals significantly to more closely match the readable stream interface.

Note: this is a __breaking__ (semver major) change to this package as the close event behavior is changed slightly, and `highWaterMark` is no longer supported.  It shouldn't impact most usage, but breaking regardless.
brianc added a commit that referenced this pull request Dec 30, 2019
* Fix pg-query-stream

There were some subtle behaviors with the stream being implemented incorrectly & not working as expected with async iteration.  I've modified the code based on #2050 and comments in #2035 to have better test coverage of async iterables and update the internals significantly to more closely match the readable stream interface.

Note: this is a __breaking__ (semver major) change to this package as the close event behavior is changed slightly, and `highWaterMark` is no longer supported.  It shouldn't impact most usage, but breaking regardless.

* Remove a bunch of additional code

* Add test for destroy + error propagation

* Add failing test for destroying unsubmitted stream

* Do not throw an uncatchable error when closing an unused cursor
@matthieusieben
Copy link
Contributor Author

Fixed by #2051

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant