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: add note about header values encoding #42624

Closed
wants to merge 2 commits into from

Conversation

ShogunPanda
Copy link
Contributor

This PR add a note about http.Outgoing.setHeader.
At the moment, if the user use .setHeader(name, value) and value contains non latin1 characters, then method will throw an exception. But this is not documented anywhere.

Additionally it also suggests to use RFC8187 standard when UTF-8 values must be passed (rather than arbitrarily convert those values to latin1 as suggested on the net).

Fixes: #42579

@ShogunPanda ShogunPanda added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Fast-track has been requested by @ShogunPanda. Please 👍 to approve.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Apr 6, 2022
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

doc/api/http.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 6, 2022
@ShogunPanda
Copy link
Contributor Author

@aduh95 I see the commit label since this morning. Is the commit queue stuck?

If that's the case I'll land this tomorrow from if you and other collaborators +1 the fast track.

@ShogunPanda ShogunPanda removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fast-track PRs that do not need to wait for 48 hours to land. labels Apr 8, 2022
@aduh95
Copy link
Contributor

aduh95 commented Apr 8, 2022

The commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label does not instruct the CQ to land the PR:

To make the Commit Queue squash all the commits of a pull request into the
first one, add the `commit-queue-squash` label.

Only the commit-queue Add this label to land a pull request using GitHub Actions. label triggers the CQ, so it's not stuck, it simply not on the queue.

From a high-level, the Commit Queue works as follow:
1. Collaborators will add `commit-queue` label to pull requests ready to land
2. Every five minutes the queue will do the following for each pull request
with the label:

You should keep commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. on this PR otherwise the CQ will refuse to land it (because it contains more than one commit after git rebase --autosquash).

@ShogunPanda
Copy link
Contributor Author

I see. Actually I was planning to land it manually using git node.

Do you mind approving it so the tool will not complain of commits after latest approvals?

ShogunPanda added a commit that referenced this pull request Apr 8, 2022
PR-URL: #42624
Fixes: #42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@ShogunPanda
Copy link
Contributor Author

Landed in dfc2dc8

@ShogunPanda ShogunPanda closed this Apr 8, 2022
@ShogunPanda ShogunPanda deleted the set-headers-rfc-8187 branch April 8, 2022 14:05
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
PR-URL: nodejs#42624
Fixes: nodejs#42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #42624
Fixes: #42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #42624
Fixes: #42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 11, 2022
PR-URL: #42624
Fixes: #42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42624
Fixes: #42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42624
Fixes: nodejs/node#42579
Co-authored-by: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
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. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Headers are unnecessary encoded to ByteString if there is no Transfer-Encoding: chunked header
4 participants