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

http2: [compat api] add writable* properties by rexagod · Pull Request #33506 · nodejs/node · GitHub

/ node Public

http2: [compat api] add writable* properties - #33506

Closed
rexagod wants to merge 3 commits into
nodejs:masterfrom
rexagod:i-29829-09-10-11
ConversationCommits (3)ChecksFiles changed
Closed

http2: [compat api] add writable* properties#33506
rexagod wants to merge 3 commits into
nodejs:masterfrom
rexagod:i-29829-09-10-11

Conversation

rexagod commented May 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: #29829 (9th, 10th, and 11th sub-issues)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

http2: [compat api] add writable* properties
4e847d9 
added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: nodejs#29829
nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label May 22, 2020
rexagod closed this May 22, 2020
rexagod reopened this May 22, 2020
ronag reviewed May 24, 2020
View reviewed changes
Comment thread lib/internal/http2/compat.js
}

get writableFinished() {
return this[kStream].writableFinished;

ronag May 24, 2020

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

I suspect this will need a special case and test for state.headRequest.

ronag May 24, 2020 •
edited
Loading

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

Though I think headRequest is slightly broken or maybe I don't understand it. There is special handling for it in Http2ServerResponse.end() but not in Http2ServerResponse.write()? Not sure who is a good ping there. @jasnell?

rexagod May 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

EDIT: please create an issue if landing this PR without resolving this.

Issue: #33543

ronag reviewed May 24, 2020
View reviewed changes

ronag left a comment •
edited
Loading

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, added a comment for state.headRequest but I would be fine with sorting that out in a follow up PR if it is difficult to resolve.

EDIT: please create an issue if landing this PR without resolving this.

mcollina approved these changes May 24, 2020
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 May 24, 2020

Copy link
Copy Markdown
Collaborator

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 24, 2020
rexagod mentioned this pull request May 24, 2020
Closed
lpinca approved these changes May 24, 2020
View reviewed changes
Comment thread test/parallel/test-http2-res-writable-properties.js Outdated
Update test/parallel/test-http2-res-writable-properties.js
a729aa1 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot commented May 25, 2020

Copy link
Copy Markdown
Collaborator

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 30, 2020
http2: add writable* properties to compat api
99abaf9 
added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: nodejs#29829

PR-URL: nodejs#33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR commented May 30, 2020

Copy link
Copy Markdown
Member

Landed in 99abaf9

BridgeAR closed this May 30, 2020
rexagod mentioned this pull request Jun 6, 2020
Closed
32 tasks
codebytere pushed a commit that referenced this pull request Jun 18, 2020
http2: add writable* properties to compat api
d8aeafb 
added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: #29829

PR-URL: #33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
http2: add writable* properties to compat api
393e894 
added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: #29829

PR-URL: #33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
http2: add writable* properties to compat api
0f0720a 
added writableHighWaterMark, writableLength, and writableFinished
properties with test.

Refs: #29829

PR-URL: #33506
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere mentioned this pull request Jul 13, 2020
Merged
trivikr mentioned this pull request May 20, 2026
Closed
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

ronag ronag left review comments
mcollina mcollina approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
apapirovski Awaiting requested review from apapirovski

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL