stream: add isDisturbed helper by ronag · Pull Request #39628 · nodejs/node

ronag · 2021-08-02T11:10:06Z

Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: #39627

mcollina

Good for me! It needs docs and tests

ronag · 2021-08-02T12:25:10Z

@mcollina added

mcollina

lgtm

nodejs-github-bot · 2021-08-02T13:09:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39416/

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: nodejs#39627

nodejs-github-bot · 2021-08-02T13:39:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39418/

ronag · 2021-08-03T07:35:08Z

@nodejs/streams this needed some additional changes to make things consistent. PTAL.

addaleax · 2021-08-03T14:29:36Z

  * `signal` {AbortSignal}
 * Returns: {stream.Readable}

+### `stream.Readable.isDisturbed(stream)`


I think the name here could be more expressive, maybe .hasEmittedStreamEvents()? I know it's generic, but so is "disturbed", and it's a bit more precise.

We should also add to the documentation that this only starts being true when data has been emitted (i.e. stream.on('data'), not when data has been requested from the underlying resource (i.e. stream._read()) or made available to the stream implementation (i.e. stream.push()).

I think the name here could be more expressive, maybe .hasEmittedStreamEvents()? I know it's generic, but so is "disturbed", and it's a bit more precise.

I disagree. It's not just when it has been read, it's also when it has been cancelled/aborted. The WHATWG spec calls this state disturbed. Why make up a different name?

We should also add to the documentation that this only starts being true when data has been emitted (i.e. stream.on('data'), not when data has been requested from the underlying resource (i.e. stream._read()) or made available to the stream implementation (i.e. stream.push()).

I think that it is kind of clear. Neither push nor _read "reads" from the Readable interface. Anyway I'm open for suggestions...

Why make up a different name?

The argument is: Because the spec naming isn't typically meant for end-users and streams are very confusing to most of our users anyway basically.

ronag · 2021-08-04T10:50:49Z

This can land per se. @mscdex @addaleax are you blocking in regards to the naming?

jasnell · 2021-08-04T12:07:44Z

I prefer to keep the current naming that is consistent with the spec language. It makes for less cognitive load when going between the two models and makes it clear that they share a common intent and purpose.

addaleax · 2021-08-04T13:48:06Z

@ronag No. I still think it’s a confusind and unhelpful name, though.

I understand @jasnell’s point, but how many people actually do interact with the spec in the way and the frequency that you do?

ronag · 2021-08-04T13:52:46Z

@ronag No. I still think it’s a confusind and unhelpful name, though.

Do you have any alternative suggestions?

I understand @jasnell’s point, but how many people actually do interact with the spec in the way and the frequency that you do?

I would expect the people that would actually use this API to be quite likely to interact with the spec...

ronag · 2021-08-04T13:54:50Z

An alternative (that the spec also uses) is bodyUsed. However, I think the spec people themselves are unhappy with that one.

olingern · 2021-08-05T13:50:00Z

Just as an onlooker to this PR, I wasn't familiar with a stream being "disturbed" until I went and read the spec, but I can understand @addaleax's suggestion, hasEmittedStreamEvents, instantly without understanding the underlying mechanism. Obvious trade-offs there.

I think this is more about ergonomics and what Node intends to provide to end users rather than clarity / good naming. Is there a set of "guiding principles" for this sort of thing?

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: #39627 PR-URL: #39628 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ronag · 2021-08-06T05:46:42Z

Landed in 4832d1c

danielleadams · 2021-08-16T14:40:28Z

@ronag Can you backport this to v16.x-staging? thanks!

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: nodejs#39627 PR-URL: nodejs#39628 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: nodejs#39627 PR-URL: nodejs#39628 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Backport-PR-URL: nodejs#39819

ronag · 2021-08-20T13:31:46Z

@danielleadams #39819

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: nodejs#39627 PR-URL: nodejs#39628 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Backport-PR-URL: nodejs#39819

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: #39627 PR-URL: #39628 Backport-PR-URL: #39819 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: doc: * deprecate type coercion for `dns.lookup` options (Antoine du Hamel) #38906 stream: * (SEMVER-MINOR) add `stream.Duplex.from` utility (Robert Nagy) #39519 * (SEMVER-MINOR) add `isDisturbed` helper (Robert Nagy) #39628 util: * (SEMVER-MINOR) expose `toUSVString` (Robert Nagy) #39814 PR-URL: #39875

Notable changes: doc: * deprecate type coercion for `dns.lookup` options (Antoine du Hamel) nodejs#38906 stream: * (SEMVER-MINOR) add `stream.Duplex.from` utility (Robert Nagy) nodejs#39519 * (SEMVER-MINOR) add `isDisturbed` helper (Robert Nagy) nodejs#39628 util: * (SEMVER-MINOR) expose `toUSVString` (Robert Nagy) nodejs#39814 PR-URL: nodejs#39875

ronag added stream Issues and PRs related to the stream subsystem. web streams labels Aug 2, 2021

ronag requested review from benjamingr, jasnell and mcollina August 2, 2021 11:10

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 2, 2021

ronag force-pushed the is-disturbed branch from 0d73c31 to 3bd9a53 Compare August 2, 2021 11:11

benjamingr approved these changes Aug 2, 2021

View reviewed changes

mcollina requested changes Aug 2, 2021

View reviewed changes

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 2, 2021

ronag requested a review from mcollina August 2, 2021 12:25

ronag force-pushed the is-disturbed branch from 3d31a46 to 8c1bdf1 Compare August 2, 2021 12:25

mcollina approved these changes Aug 2, 2021

View reviewed changes

ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Aug 2, 2021

Trott force-pushed the master branch from 6ca23d7 to ac621ff Compare August 2, 2021 13:01

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2021

mscdex reviewed Aug 2, 2021

View reviewed changes

Comment thread doc/api/stream.md Outdated

ronag added 2 commits August 2, 2021 15:36

stream: add isDisturbed helper

d363920

Adds a helper util used to determine whether a stream has been disturbed (read or cancelled). Refs: nodejs#39627

fixup

ac0a1df

ronag force-pushed the is-disturbed branch from 8c1bdf1 to ac0a1df Compare August 2, 2021 13:36

olingern reviewed Aug 3, 2021

View reviewed changes

Comment thread lib/internal/streams/readable.js Outdated

fixup

8673e33

ronag requested review from benjamingr and mcollina August 3, 2021 07:34

addaleax reviewed Aug 3, 2021

View reviewed changes

benjamingr approved these changes Aug 4, 2021

View reviewed changes

ronag added dont-land-on-v12.x labels Aug 4, 2021

ronag closed this Aug 6, 2021

danielleadams added the backport-requested-v16.x label Aug 16, 2021

ronag self-assigned this Aug 16, 2021

ronag added the backport-open-v16.x label Aug 20, 2021

targos removed the backport-requested-v16.x label Aug 22, 2021

targos mentioned this pull request Aug 22, 2021

[v16.x backport] stream: add stream.compose #39563

Closed

targos added backported-to-v16.x and removed backport-open-v16.x labels Aug 23, 2021

targos mentioned this pull request Aug 25, 2021

v16.8.0 release proposal #39875

Merged

Uh oh!

Conversation

ronag commented Aug 2, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 2, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 2, 2021

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 2, 2021

Uh oh!

Uh oh!

ronag commented Aug 3, 2021

Uh oh!

addaleax Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

ronag Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 4, 2021

Uh oh!

jasnell commented Aug 4, 2021

Uh oh!

addaleax commented Aug 4, 2021

Uh oh!

ronag commented Aug 4, 2021

Uh oh!

ronag commented Aug 4, 2021

Uh oh!

olingern commented Aug 5, 2021

Uh oh!

ronag commented Aug 6, 2021

Uh oh!

danielleadams commented Aug 16, 2021

Uh oh!

ronag commented Aug 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ronag Aug 3, 2021 •

edited

Loading