GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||
GeezerVison is showing github.com — http: join all duplicate headers by marco-ippolito · Pull Request #45982 · nodejs/node Conversation
|
|
Review requested:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on adding a CLI flag. This changes behavior of existing code in a potentially sensitive way, and users do not necessarily know which parts of their application require current behavior or not.
I think the suggestion from the ticket is actually way better (and as I'm spelling this out, I realize it's your own suggestion 🙂), which is to add a flag to the HTTP server/client options instead.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@addaleax yes it was my suggestion to use the option in server/client 😄. While working on it I realized it was very messy and produced unexpected behavior (the option missing from server but present in the client) so I switched to a flag which seemed to me a cleaner and "universal" solution. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@marco-ippolito How does using a CLI flag solve that? Typically, client and server won't be running in the same process or be even developed/deployed by the same person, so you'll end up with a good chance of mismatches for this option either way. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This should be an option, not a CLI flag. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
ok changing it to option. Should I mark it as experimental in the docs? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Actually, RFC 9110 requires multiple occurrences of all comma-separated headers to be joinable. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Blocking and requesting changes because the documentation is wrong.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
See: #46240. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: #45982 Backport-PR-URL: #46240 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
Notable changes: * crypto: * (SEMVER-MINOR) add CryptoKey Symbol.toStringTag (Filip Skokan) [#46042](#46042) * (SEMVER-MINOR) add KeyObject Symbol.toStringTag (Filip Skokan) [#46043](#46043) * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: #46286
Notable changes: * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: #46286 Signed-off-by: RafaelGSS <[email protected]>
Notable changes: * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: #46286 Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #45982 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: #45982 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396
Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982 PR-URL: nodejs#48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982 PR-URL: nodejs#48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
DuplicateAuthorization headers should not be ignored.
There was an error while loading. Please reload this page.