Skip to content

doc: return type is number - #29828

Closed
exoego wants to merge 1 commit into
nodejs:masterfrom
exoego:http2-doc
Closed

doc: return type is number#29828
exoego wants to merge 1 commit into
nodejs:masterfrom
exoego:http2-doc

Conversation

@exoego

@exoegoexoego commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

node/test/parallel/test-http2-session-stream-state.js

Lines 22 to 23 in 88ef086

assert.strictEqual(typeofstate.localClose,'number');
assert.strictEqual(typeofstate.remoteClose,'number');

Type of localClose and remoteClose are number, so I think 1 is valid instead of true.

Checklist

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Oct 3, 2019
Trott
Trott reviewed Oct 3, 2019
View reviewed changes
Comment threaddoc/api/http2.md
* `localClose` {number} `true` if this `Http2Stream` has been closed locally.
* `remoteClose` {number} `true` if this `Http2Stream` has been closed
* `localClose` {number} `1` if this `Http2Stream` has been closed locally.
* `remoteClose` {number} `1` if this `Http2Stream` has been closed

@TrottTrottOct 3, 2019•
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.

If I'm reading the nghttp2 source correctly, it's 1 if remote peer half closed the given stream, 0 if it did not, and -1 if no such stream exists. Same for localClose. Is it possible to get -1 for these properties in Node.js?

Once the possible values are determined, tests should be augmented to check those values are returned in the expected situations. That doesn't have to happen in this PR but it would be a nice addition for sure.

@TrottTrottOct 6, 2019

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.

@nodejs/http2

Trott
Trott approved these changes Oct 11, 2019
View reviewed changes

@TrottTrott 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.

Despite my suggested improvement, this is an improvement as it is already so I'm going to land it. My additional suggestions can happen some other time (or not).

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott

Trott commented Oct 11, 2019

Copy link
Copy Markdown
Member

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

@Trott

Trott commented Oct 11, 2019

Copy link
Copy Markdown
Member

Landed in 3aeae8d

@TrottTrott closed this Oct 11, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 11, 2019
exoego@Trott
doc: return type is number
3aeae8d
PR-URL: nodejs#29828
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Oct 14, 2019
exoego@targos
doc: return type is number
f384289
PR-URL: #29828
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos mentioned this pull request Oct 15, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@MylesBorinsMylesBorinsAwaiting requested review from MylesBorins
@mcollinamcollinaAwaiting requested review from mcollina
@trivikrtrivikrAwaiting requested review from trivikr
@apapirovskiapapirovskiAwaiting requested review from apapirovski

Assignees

No one assigned

Labels

docIssues and PRs related to the documentations.http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@exoego@nodejs-github-bot@Trott