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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Oct 9, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Trott commented Oct 12, 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
Copy link
Member

@lpinca lpinca Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronag
Copy link
Member Author

ronag commented Nov 17, 2019

Maybe a quick review from @mcollina?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, good note.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 17, 2019
doc/api/stream.md Outdated Show resolved Hide resolved
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 19, 2019
Co-Authored-By: Ruben Bridgewater <ruben@bridgewater.de>
@ronag
Copy link
Member Author

ronag commented Nov 24, 2019

@Trott: I believe this can land?

@ronag
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 lundibundi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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
doc/api/stream.md Outdated Show resolved Hide resolved
@Trott
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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 25, 2019
Trott pushed 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
Copy link
Member

Trott commented Nov 25, 2019

Landed in d67c377

@Trott Trott closed this Nov 25, 2019
addaleax pushed 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 pushed 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 pushed 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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants