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

feat(ext/fetch): add Headers#getSetCookie #13542

Merged
merged 4 commits into from
Jul 2, 2023

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Jan 30, 2022

Spec change: whatwg/fetch#1346
Tests: web-platform-tests/wpt#31442 (ran against this PR and they all pass)

@stale
Copy link

stale bot commented Apr 2, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 2, 2022
@stale stale bot closed this Apr 9, 2022
@andreubotella
Copy link
Contributor

Now that the spec PR is about to land, this PR should be reopened.

ext/fetch/20_headers.js Outdated Show resolved Hide resolved
@andreubotella
Copy link
Contributor

andreubotella commented Feb 9, 2023

Hey, aside from web-platform-tests/wpt#31442, I also opened web-platform-tests/wpt#38437 with some extra tests to address some of the review feedback from the Webkit implementation. Note that the "Set-Cookie is a forbidden response header" test is about browser behavior and doesn't apply to Deno.

@lucacasonato lucacasonato marked this pull request as ready for review February 23, 2023 16:44
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

This should be rebased and landed. Spec changes have gone through and I presume this would be a fairly important feature for eg. Node compat.

@e3dio
Copy link

e3dio commented May 24, 2023

Tried to use this today, was missing in Deno

@andreubotella
Copy link
Contributor

For the record, this implementation is correct according to the spec as it is now. The only changes in the spec text since were editorial. There is one failing test in fetch/api/headers/header-setcookie.any.js, that for Set-Cookie being a forbidden response header, that Deno fails intentionally (and which will eventually be standardized by the fetch work for server-side runtimes in WinterCG). So the only thing that's blocking this PR from landing is resolving the merge conflicts.

@bartlomieju bartlomieju added this to the 1.35 milestone May 26, 2023
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit d8e8e60 into denoland:main Jul 2, 2023
@bartlomieju bartlomieju deleted the fetch_getsetheaders branch July 2, 2023 11:21
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.

6 participants