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 readableDidRead #36820

Closed
wants to merge 3 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 6, 2021

Adds readableDidRead to streams and applies usage to http.

@ronag ronag added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. http2 Issues or PRs related to the http2 subsystem. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jan 6, 2021
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2021
@ronag ronag requested a review from lpinca January 6, 2021 20:45
@ronag ronag removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2021
@ronag ronag marked this pull request as draft January 6, 2021 21:40
@ronag ronag force-pushed the http-manualStart branch 3 times, most recently from 36596e7 to 823908d Compare January 6, 2021 22:32
@ronag

This comment has been minimized.

@ronag ronag force-pushed the http-manualStart branch 2 times, most recently from eae9165 to f89ab33 Compare January 6, 2021 22:42
@ronag ronag changed the title stream: add manualStart stream: add readableDidRead Jan 6, 2021
@ronag ronag marked this pull request as ready for review January 6, 2021 22:44
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
@ronag ronag marked this pull request as draft January 6, 2021 22:59
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
@nodejs-github-bot
Copy link
Collaborator

@ronag ronag marked this pull request as ready for review January 6, 2021 23:08
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed http2 Issues or PRs related to the http2 subsystem. labels Jul 8, 2021
@ronag
Copy link
Member Author

ronag commented Jul 8, 2021

@benjamingr @vweevers

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 11, 2021
@ronag ronag requested a review from mcollina July 13, 2021 21:53
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

@nodejs-github-bot
Copy link
Collaborator

ronag added a commit that referenced this pull request Jul 14, 2021
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: #36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ronag
Copy link
Member Author

ronag commented Jul 14, 2021

Landed in 8306051

@ronag ronag closed this Jul 14, 2021
targos pushed a commit that referenced this pull request Jul 17, 2021
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: #36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
@targos
Copy link
Member

targos commented Jul 27, 2021

Is it intended to be public API? In that case it should be documented and this PR is semver-minor.

@targos
Copy link
Member

targos commented Jul 29, 2021

@ronag ^

@ronag ronag added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 29, 2021
ronag added a commit to nxtedition/node that referenced this pull request Jul 29, 2021
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ronag
Copy link
Member Author

ronag commented Jul 30, 2021

Added don't land tags for now due to ongoing discussions.

ronag added a commit to nxtedition/node that referenced this pull request Aug 1, 2021
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos
Copy link
Member

targos commented Aug 22, 2021

Where are the discussions happening?

@targos
Copy link
Member

targos commented Aug 22, 2021

Oh I see, it was reverted and implemented differently in #39589

ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2021
Adds readableDidRead to streams and applies usage to http, http2 and quic.

PR-URL: nodejs#36820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants