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

Implement headers.getSetCookie() #321

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 26, 2023

Implementation of the proposed headers.getSetCookie() API

The API is still not 100% finished so we likely don't want to land just yet. Deno folks have asked (through wintercg) if we could coordinate on the release of this so that it hits multiple runtimes at the same time, allowing us to get a bit more of a bang-for-the-buck announcing the common API. The spec change has landed, this should be just about ready to go.

Note: this is potentially a breaking change due to the change in the way the iterator is handled. Previously, our iterator would inappropriate munge set-cookie headers together. This change separates them properly for both entries() and values(). Do we need a compatibility flag tor this behavior change? Added a compatibility flag

Refs: whatwg/fetch#1346

@jasnell jasnell requested a review from harrishancock January 26, 2023 18:27
@jasnell
Copy link
Member Author

jasnell commented Jan 26, 2023

/cc @lucacasonato

@harrishancock
Copy link
Collaborator

harrishancock commented Jan 31, 2023

This change separates them properly for both entries() and values().

The linked Fetch spec PR appears to also require us to change keys(), I think.

Specifically, the linked PR redefines the "sort and combine" operation to produce a list like (in JSON-ish pseudocode) [{ key: "set-cookie", value: "value0" }, { key: "set-cookie", value: "value1" }].

The "sort and combine" operation is used in exactly one place: to produce the "value pairs to iterate over". Thus, the "value pairs to iterate over" are a list of header names and values, with potentially-duplicate names.

In Web IDL, all four iteration methods (keys(), values(), entries(), and forEach()) are implemented generically in terms of this list of "value pairs to iterate over". My reading of the implementation suggests we're not supposed to be de-duplicating the keys, which means our current Headers::keys() implementation would be non-conformant.

Do we need a compatibility flag tor this behavior change?

I would err on the side of yes, though I don't have a particularly compelling problem scenario.

@jasnell
Copy link
Member Author

jasnell commented Jan 31, 2023

The linked Fetch spec PR appears to also require us to change keys(), I think.

@lucacasonato ... can you confirm if this is intentional? It seems odd that the keys() iterator may not be unique.

@lucacasonato
Copy link

Mh, this is an interesting case. Can you comment on the spec PR?

@jasnell jasnell force-pushed the jsnell/implement-headers-getsetcookie branch from 5fd2659 to e034a2a Compare February 3, 2023 00:29
Copy link
Collaborator

@harrishancock harrishancock left a comment

Choose a reason for hiding this comment

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

LGTM; remember to squash.

@jasnell
Copy link
Member Author

jasnell commented Feb 3, 2023

Just waiting on the spec change to land upstream before landing.

@jasnell jasnell force-pushed the jsnell/implement-headers-getsetcookie branch 2 times, most recently from 42d1f43 to 1fa1ddd Compare February 9, 2023 18:23
@jasnell jasnell requested a review from harrishancock February 9, 2023 19:12
@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2023

@harrishancock ... updated to add the compat flag, which meant also having to update the JSG_ITERATOR macro and the various places it is used. Can I ask you to take another look?

src/workerd/api/http.c++ Outdated Show resolved Hide resolved
Copy link
Collaborator

@harrishancock harrishancock left a comment

Choose a reason for hiding this comment

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

It LGTM, though I agree with Kenton that we could probably just delegate from getAll() to getSetCookie().

@jasnell jasnell force-pushed the jsnell/implement-headers-getsetcookie branch from 3bc4fe7 to 7b7fd2e Compare February 13, 2023 22:30
@jasnell
Copy link
Member Author

jasnell commented Feb 13, 2023

Updated to delegate getAll to getSetCookie, also squashed and rebased for a final CI run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants