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

doc: add notes about non-conforming streams #29895

Closed
wants to merge 8 commits into from

Conversation

@ronag
Copy link
Member

ronag commented Oct 9, 2019

A lot of streams in the ecosystem are overriding public stream methods that lead to very subtle and hard to find bugs. Discourage this usage and encourage using the existing framework.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@ronag ronag force-pushed the nxtedition:notes-about-override branch from 6c7173d to 7ce4453 Oct 9, 2019
Copy link
Member

BridgeAR left a comment

I am in general fine with the note but isn't this a general problem we have? There are still lots of "internal" APIs used and or overridden in the wild.

We also do not have "internal" events, so it's even more difficult to prevent any such usage.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 9, 2019

t isn't this a general problem we have? There are still lots of "internal" APIs used and or overridden in the wild.

Yes, nothing we can do about that... but we can still discourage it so future users are able to implement streams as correctly as possible?

We also do not have "internal" events, so it's even more difficult to prevent any such usage.

What do you mean? 'error'? It's javascript we can't prevent some stuff but at least users should be aware of what they should and should not do.

I don't think most people that override public methods are aware of the problems it might cause.

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 12, 2019

typo in commit message: s/confirming/conforming/

doc/api/stream.md Outdated Show resolved Hide resolved
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 12, 2019

Text looks fine to me but would prefer reviews from @nodejs/streams to mine.

@Trott Trott changed the title doc: add notes about non-confirming streams doc: add notes about non-conforming streams Oct 12, 2019
@Trott

This comment has been minimized.

@lpinca
lpinca approved these changes Oct 20, 2019
@@ -1667,6 +1667,12 @@ of a stream that are intended for use by consumers (as described in the
[API for Stream Consumers][] section). Doing so may lead to adverse side effects
in application code consuming the stream.

It is highly discouraged to override any public method or to emit
`'error'` events manually through `.emit(err)` instead of using API-provided

This comment has been minimized.

Copy link
@lpinca

lpinca Oct 20, 2019

Member

We also do not have "internal" events, so it's even more difficult to prevent any such usage.

I think that @BridgeAR was referring to events like 'data', 'finish', 'end', 'close', etc. not only 'error'.

This comment has been minimized.

Copy link
@ronag

ronag Oct 20, 2019

Author Member

I see. Not sure what to do with that though? Should I add "internal events" to the paragraph?

@ronag ronag force-pushed the nxtedition:notes-about-override branch from a963259 to e2207eb Oct 20, 2019
@ronag ronag force-pushed the nxtedition:notes-about-override branch from 3c45c5a to 1ac15d1 Oct 20, 2019
@ronag ronag requested review from BridgeAR and Trott Nov 17, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Nov 17, 2019

Maybe a quick review from @mcollina?

Copy link
Member

mcollina left a comment

lgtm, good note.

@Trott Trott added the author ready label Nov 17, 2019
doc/api/stream.md Outdated Show resolved Hide resolved
@addaleax addaleax removed the author ready label Nov 19, 2019
Co-Authored-By: Ruben Bridgewater <ruben@bridgewater.de>
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Nov 24, 2019

@Trott: I believe this can land?

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Nov 24, 2019

Maybe @lundibundi & @yorkie has some last minute feedback?

doc/api/stream.md Outdated Show resolved Hide resolved
Copy link
Member

lundibundi left a comment

👍

doc/api/stream.md Outdated Show resolved Hide resolved
@yorkie
yorkie approved these changes Nov 24, 2019
@ronag ronag force-pushed the nxtedition:notes-about-override branch from 9a3f364 to fdba619 Nov 24, 2019
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 25, 2019

I left a bunch of small suggestions, but none of them are blocking.

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4087/

@Trott Trott added the author ready label Nov 25, 2019
ronag and others added 3 commits Nov 25, 2019
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@Trott
Trott approved these changes Nov 25, 2019
Trott added a commit that referenced this pull request Nov 25, 2019
PR-URL: #29895
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 25, 2019

Landed in d67c377

@Trott Trott closed this Nov 25, 2019
addaleax added a commit that referenced this pull request Nov 30, 2019
PR-URL: #29895
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Dec 1, 2019
PR-URL: #29895
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins added a commit that referenced this pull request Dec 17, 2019
PR-URL: #29895
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.