Skip to content

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

Merged
bartlomieju merged 4 commits into
denoland:mainfrom
lucacasonato:fetch_getsetheaders
Jul 2, 2023
Merged

feat(ext/fetch): add Headers#getSetCookie#13542
bartlomieju merged 4 commits into
denoland:mainfrom
lucacasonato:fetch_getsetheaders

Conversation

@lucacasonato

@lucacasonato lucacasonato commented Jan 30, 2022

Copy link
Copy Markdown
Contributor

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

@stale

stale Bot commented Apr 2, 2022

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

Comment thread ext/fetch/20_headers.js Outdated
@andreubotella

andreubotella commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

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

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@aapoalas aapoalas left a comment

Copy link
Copy Markdown
Contributor

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

e3dio commented May 24, 2023

Copy link
Copy Markdown

Tried to use this today, was missing in Deno

@andreubotella

Copy link
Copy Markdown
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

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

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