Skip to content

Conversation

@Flarna
Copy link
Member

@FlarnaFlarna commented Jun 15, 2018

Incoming set-cookie headers should be passed to user as array like in http module - even for single occurrence.

Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code.

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

Regarding doc: If this PR is accepted #21296 should be updated accordingly - or doc needs to be adapted in this PR if #21296 landed meanwhile.

@ryzokuken
Copy link
Contributor

/cc @nodejs/http2

@ryzokukenryzokuken added the http2 Issues or PRs related to the http2 subsystem. label Jun 16, 2018
@ryzokuken
Copy link
Contributor

@maclover7 @nodejs/github-bot any idea why neither the CI nor labelling script in the bot was triggered for this one? It seems rather strange.

@joyeecheung
Copy link
Member

@ryzokuken The bot is down now: nodejs/build#1353

@BridgeAR
Copy link
Member

@nodejs/http2 PTAL

Copy link
Contributor

@apapirovskiapapirovski left a comment

Choose a reason for hiding this comment

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

LGTM but I would prefer if we could just tack this test on to one of the existing headers tests rather than creating a new one for such a small behaviour.

@Flarna
Copy link
MemberAuthor

Flarna commented Jun 19, 2018

@apapirovski ok, will try to move the test. I have chosen the easiest way to reproduce this 😁
Done: Removed new test, added checks in existing test-http2-util-headers-list.js

Copy link
Contributor

@apapirovskiapapirovski left a comment

Choose a reason for hiding this comment

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

LGTM with a small nit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a dot at the end of this sentence, please?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure, will do that soon. As #21296 landed I have to update doc for set-cookie also.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Done

@apapirovski
Copy link
Contributor

@apapirovskiapapirovski 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
Contributor

Weird failures last time. CI: https://ci.nodejs.org/job/node-test-pull-request/15606/

@Flarna
Copy link
MemberAuthor

Flarna commented Jun 25, 2018

@apapirovski Looks like CI got confused because I did a git rebase. It tries to checkout commit 05c0f78b79cb7682c51e49af57341baa3f8d94db which was the last commit before the rebase but it should checkout ada2076d4ef919bc6d2981c39174c28a826fa971.

Applies at least to some of the CI jobs.

@apapirovski
Copy link
Contributor

Was there a merge commit by any chance? Weird that this is happening. Could you maybe squash?

Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code.
@Flarna
Copy link
MemberAuthor

No, it was a "normal" rebase. Rebased again and squashed.

@Flarna
Copy link
MemberAuthor

Is there anything I should add here?

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

@mcollina
Copy link
Member

Landed in d4966a1

@mcollinamcollina closed this Jul 5, 2018
mcollina pushed a commit that referenced this pull request Jul 5, 2018
Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. PR-URL: #21360 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@FlarnaFlarna deleted the http2_set-cookie_array branch July 5, 2018 13:45
targos pushed a commit that referenced this pull request Jul 6, 2018
Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. PR-URL: #21360 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@targostargos mentioned this pull request Jul 17, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. PR-URL: nodejs#21360 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. PR-URL: nodejs#21360 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. PR-URL: nodejs#21360 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. Backport-PR-URL: #22850 PR-URL: #21360 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Oct 30, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@Flarna@ryzokuken@joyeecheung@BridgeAR@apapirovski@mcollina@jasnell@TimothyGu@trivikr