FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: add readableDidRead by ronag · Pull Request #36820 · nodejs/node · GitHub

/ node Public

stream: add readableDidRead - #36820

Closed
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:http-manualStart
ConversationCommits (3)ChecksFiles changed
Closed

stream: add readableDidRead#36820
ronag wants to merge 3 commits into
nodejs:masterfrom
nxtedition:http-manualStart

Conversation

ronag commented Jan 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Adds readableDidRead to streams and applies usage to http.

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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2021
ronag force-pushed the http-manualStart branch from 0fbd1ca to d1956d1 Compare January 6, 2021 20:42
ronag requested a review from lpinca January 6, 2021 20:45
ronag removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 6, 2021
mscdex reviewed Jan 6, 2021
View reviewed changes
Comment thread test/parallel/test-stream-readable-manualStart.js Outdated
mscdex reviewed Jan 6, 2021
View reviewed changes
Comment thread test/parallel/test-stream-readable-manualStart.js Outdated
mscdex reviewed Jan 6, 2021
View reviewed changes
Comment thread test/parallel/test-stream-readable-manualStart.js Outdated
ronag marked this pull request as draft January 6, 2021 21:40
ronag force-pushed the http-manualStart branch 3 times, most recently from 36596e7 to 823908d Compare January 6, 2021 22:32

This comment has been minimized.

Sign in to view
ronag force-pushed the http-manualStart branch 2 times, most recently from eae9165 to f89ab33 Compare January 6, 2021 22:42
ronag changed the title stream: add manualStart stream: add readableDidRead Jan 6, 2021
ronag force-pushed the http-manualStart branch from f89ab33 to bbdf58e Compare January 6, 2021 22:43
ronag marked this pull request as ready for review January 6, 2021 22:44
ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
ronag marked this pull request as draft January 6, 2021 22:59
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 commented Jan 6, 2021

Copy link
Copy Markdown
Collaborator

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

ronag force-pushed the http-manualStart branch from bbe60f9 to 126dd80 Compare January 6, 2021 23:06
ronag marked this pull request as ready for review January 6, 2021 23:08
ronag added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed http2 Issues or PRs related to the http2 subsystem. labels Jul 8, 2021

ronag commented Jul 8, 2021

Copy link
Copy Markdown
Member Author

@benjamingr @vweevers

ronag mentioned this pull request Jul 9, 2021
Closed
targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 11, 2021
mcollina reviewed Jul 13, 2021
View reviewed changes
Comment thread lib/_http_server.js
ronag requested a review from mcollina July 13, 2021 21:53
mcollina approved these changes Jul 14, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

nodejs-github-bot commented Jul 14, 2021

Copy link
Copy Markdown
Collaborator

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

ronag added a commit that referenced this pull request Jul 14, 2021
stream: add readableDidRead
8306051 
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 commented Jul 14, 2021

Copy link
Copy Markdown
Member Author

Landed in 8306051

ronag closed this Jul 14, 2021
targos pushed a commit that referenced this pull request Jul 17, 2021
stream: add readableDidRead
8fe7d23 
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 mentioned this pull request Jul 26, 2021
Merged

targos commented Jul 27, 2021

Copy link
Copy Markdown
Member

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

targos commented Jul 29, 2021

Copy link
Copy Markdown
Member

@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 commented Jul 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

targos commented Aug 22, 2021

Copy link
Copy Markdown
Member

Where are the discussions happening?

targos commented Aug 22, 2021

Copy link
Copy Markdown
Member

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

BethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
benjamingr benjamingr approved these changes
apapirovski Awaiting requested review from apapirovski
lpinca Awaiting requested review from lpinca
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL