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

stream: add isDisturbed helper #39628

Closed
wants to merge 6 commits into from
Closed

Conversation

@ronag
Copy link
Member

@ronag ronag commented Aug 2, 2021

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

Refs: #39627

Copy link
Member

@mcollina mcollina left a comment

Good for me! It needs docs and tests

@ronag
Copy link
Member Author

@ronag ronag commented Aug 2, 2021

@mcollina added

@ronag ronag requested a review from mcollina Aug 2, 2021
@ronag ronag force-pushed the is-disturbed branch from 3d31a46 to 8c1bdf1 Aug 2, 2021
Copy link
Member

@mcollina mcollina left a comment

lgtm

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 2, 2021

doc/api/stream.md Outdated Show resolved Hide resolved
ronag added 2 commits Aug 2, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627
@ronag ronag force-pushed the is-disturbed branch from 8c1bdf1 to ac0a1df Aug 2, 2021
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 2, 2021

lib/internal/streams/readable.js Outdated Show resolved Hide resolved
@ronag ronag requested review from benjamingr and mcollina Aug 3, 2021
@ronag
Copy link
Member Author

@ronag ronag commented Aug 3, 2021

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

@@ -2046,6 +2057,18 @@ added: REPLACEME
* `signal` {AbortSignal}
* Returns: {stream.Readable}

### `stream.Readable.isDisturbed(stream)`
Copy link
Member

@addaleax addaleax Aug 3, 2021

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()).

Copy link
Member Author

@ronag ronag Aug 3, 2021

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...

Copy link
Member

@benjamingr benjamingr Aug 4, 2021

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
Copy link
Member Author

@ronag ronag commented Aug 4, 2021

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

@jasnell
Copy link
Member

@jasnell jasnell commented Aug 4, 2021

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
Copy link
Member

@addaleax addaleax commented Aug 4, 2021

@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
Copy link
Member Author

@ronag ronag commented Aug 4, 2021

@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
Copy link
Member Author

@ronag ronag commented Aug 4, 2021

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

@olingern
Copy link

@olingern olingern commented Aug 5, 2021

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?

ronag added a commit that referenced this issue Aug 6, 2021
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
Copy link
Member Author

@ronag ronag commented Aug 6, 2021

Landed in 4832d1c

@danielleadams
Copy link
Member

@danielleadams danielleadams commented Aug 16, 2021

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

@ronag ronag self-assigned this Aug 16, 2021
ronag added a commit to nxtedition/node that referenced this issue Aug 20, 2021
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>
ronag added a commit to nxtedition/node that referenced this issue Aug 20, 2021
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
Copy link
Member Author

@ronag ronag commented Aug 20, 2021

ronag added a commit to nxtedition/node that referenced this issue Aug 20, 2021
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
targos added a commit that referenced this issue Aug 23, 2021
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>
targos added a commit that referenced this issue Aug 25, 2021
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
targos added a commit that referenced this issue Aug 25, 2021
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
wwwzbwcom added a commit to wwwzbwcom/node that referenced this issue Aug 26, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants