Adds ability to add more than one cookie per response #1161
Conversation
merwan7
force-pushed
the
mr-set-cookie-array
branch
2 times, most recently
from
45e6a9f to
25c153c
Compare
|
setting this back to draft while I figure out testing |
merwan7
changed the title
for of instead of Object.fromEntries to preserve arrays in headers
blittle
force-pushed
the
mr-set-cookie-array
branch
from
6513244 to
2e2eb22
Compare
| const rawHeaders = headers.raw(); | ||
| // Warning! Headers.raw is non-standard and might disappear in undici or newer versions of node-fetch | ||
| // See: https://github.com/whatwg/fetch/issues/973 | ||
| const setCookieKey = Object.keys(rawHeaders).find( |
There was a problem hiding this comment.
The problem here is that headers.entries() concatenates multiple set-cookie headers into a single string separated by a comma. The thing is that cookie values can have commas, so the browser mis-interprets the multiple cookies. headers.raw() is an non-standard API that returns an array for the value. { 'set-cookie': ['someval=5', 'someotherval=1']}. Subsequently we can use that array on line 947 to append multiple Set-Cookie headers.
There was a problem hiding this comment.
Do we want to limit this to set-cookie? Do we need to support setting multiple values for all headers?
There was a problem hiding this comment.
This was discussed in whatwg/fetch#973, and it doesn't seem to be a problem with other headers. The downside for doing it with all headers is that it just adds more bytes over the wire.
There was a problem hiding this comment.
Specifically non Set-Cookie headers:
values may be comma-concatenated anyway by any intermediate proxy
| const rawHeaders = headers.raw(); | ||
| // Warning! Headers.raw is non-standard and might disappear in undici or newer versions of node-fetch | ||
| // See: https://github.com/whatwg/fetch/issues/973 | ||
| const setCookieKey = Object.keys(rawHeaders).find( |
There was a problem hiding this comment.
Do we want to limit this to set-cookie? Do we need to support setting multiple values for all headers?
* v1.x-2022-07: (95 commits) [ci] release v1.x-2022-07 (#1170) Try ignoring hello-world to see if it will get bumped Don't consider examples part of the workspace (#1202) Fix headers on oxygen (#1201) Add bot user agents for Seoradar and Adresults, resolves #1199 (#1200) Fix changeset updates to docker deploy documentation to resolve run issues (#1196) Upgrade body-parser (#1162) Fix path for deployments Adds ability to add more than one cookie per response (#1161) Move Demo Store to templates folder (#1132) Avoid additional div element (#1191) Whoops this should only be patch Adds preconnect <link> for CDN (#1160) Bump ejs from 3.1.6 to 3.1.7 (#1147) Fix scroll restoration when server props are changed (#1152) Typo Fixes #1165 by making a missing alt tag a console warning (#1167) Remove concurrency directive for Oxygen deployments Fix hydrogen-ui dev and build issues (#1169) ...
Description
This is necessary because the only way to set multiple cookies reliably from node is by writing a header like this:
We need to use
headers.raw()which is currently provided bynode-fetch, so that we can access the unserialized array and set it on the response. Otherwisenode-fetch's implementation ofHeaders, which used by hydrogen, will serialize the array into a comma separated string.Makes me think that now that fetch is part of Node v16 and v18 this may stop being a problem (I haven't confirmed this yet), but while
node-fetch's Response is in use, it's best to useheaders.raw()instead for this purpose.Additional context
This fixes this minor bug Shopify/hydrogen#1138 which we need for our brochure site so we can set our attribution and other cookies in one response.
This behavior of node-fetch is described in its readme here: https://github.com/node-fetch/node-fetch#extract-set-cookie-header
Before submitting the PR, please make sure you do the following:
fixes #123)yarn changeset addif this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning