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: Improve doc for Http2 headers object #21296

Closed
wants to merge 3 commits into from
Closed

doc: Improve doc for Http2 headers object #21296

wants to merge 3 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jun 12, 2018

Add more details regarding processing and data type of incoming headers in Http2.

Checklist

Add more details regarding processing and data type of incoming
headers in Http2.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Jun 12, 2018
@Trott
Copy link
Member

Trott commented Jun 12, 2018

@nodejs/documentation

@apapirovski
Copy link
Member

I'm not convinced most of this belongs in the http2 documentation since we're just following the spec (and it applies to http too). I am potentially in favour of documenting the type of the :status header.

@Flarna
Copy link
Member Author

Flarna commented Jun 13, 2018

@apapirovski My start point in this was the :status header as I was surprised that it is no string. By looking closer into the code and http docs I found subtle differences therefore I thought it's better to write down all details - similar as in http (see https://nodejs.org/dist/latest/docs/api/http.html#http_message_headers).
I found following differences:

  • The list of headers where duplicates is discarded differs between http and http2 implementation
  • set-cookie is always an array for http, in http2 only if there are more values

I'm fine with removing some of the details, replace it by references to http or http2 spec (or even code).
Or maybe these differences are actually bugs?

doc/api/http2.md Outdated
discarded.
* `set-cookie` is a string if present once or an array in case duplicates
are present.
* `cookie` the values are joined together with '; '.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a colon after `cookie`?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Flarna
Copy link
Member Author

Flarna commented Jun 14, 2018

I was thinking once more regarding the difference in handling of set-cookie between http2 and http. This effects also the http2 compat API so it is not that compatible as it could be.

Till now this diff is not documented and http2 is experimental so this gap could be closed easily.

@nodejs/http2 any opinion in this?

@Flarna
Copy link
Member Author

Flarna commented Jun 14, 2018

@apapirovski What do think about following: Duplicates where only a single value is allowed (e.g. content-type, age,...) are discarded.?
Still clarifies that duplicates are discarded but without the burden to keep doc and code in sync.

@apapirovski
Copy link
Member

@Flarna I would be fine with that but this has approvals as is so don't feel pressured to make the change. I think I'm in the minority in my opinion :)

@Flarna
Copy link
Member Author

Flarna commented Jun 15, 2018

@apapirovski I took a fast look into the history and it seems the headerlist is unchanged since ~9 month. Therefore I think it's ok to mention all headers in doc as the risk of getting out of sync is low.

@vsemozhetbyt
Copy link
Contributor

Let's land this, say, tomorrow if there are no strong objections?

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2018
@apapirovski
Copy link
Member

Landed in 8132413

apapirovski pushed a commit that referenced this pull request Jun 25, 2018
Add more details regarding processing and data type of incoming
headers in Http2.

PR-URL: #21296
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@Flarna Flarna deleted the doc_http2_incoming_headers branch June 25, 2018 07:18
targos pushed a commit that referenced this pull request Jun 25, 2018
Add more details regarding processing and data type of incoming
headers in Http2.

PR-URL: #21296
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Add more details regarding processing and data type of incoming
headers in Http2.

PR-URL: nodejs#21296
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
Add more details regarding processing and data type of incoming
headers in Http2.

PR-URL: nodejs#21296
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Add more details regarding processing and data type of incoming
headers in Http2.

PR-URL: nodejs#21296
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Add more details regarding processing and data type of incoming
headers in Http2.

Backport-PR-URL: #22850
PR-URL: #21296
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants