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

add doc for Headers.getSetCookie() #26161

Merged
merged 6 commits into from
Apr 18, 2023

Conversation

chrisdavidmills
Copy link
Contributor

@chrisdavidmills chrisdavidmills commented Apr 12, 2023

Description

The Headers.getSetCookie() method has been added to Chrome 113. This PR adds a reference document for this new method.

This is a pretty straightforward addition. You can find other useful links in my research document.

One question I had — the Set-Cookie header page includes a note:

Warning: Browsers block frontend JavaScript code from accessing the Set-Cookie header, as required by the Fetch spec, which defines Set-Cookie as a forbidden response-header name that must be filtered out from any response exposed to frontend code.

This sounds contradictory to the purpose of getSetCookie() — has this note interpreted the spec correctly, and if so, should I update it to list getSetCookie() as an exception to this rule?

Motivation

Additional details

Related issues and pull requests

Fix #24266

@chrisdavidmills chrisdavidmills requested a review from a team as a code owner April 12, 2023 15:44
@chrisdavidmills chrisdavidmills requested review from Elchi3 and removed request for a team April 12, 2023 15:44
@github-actions github-actions bot added the Content:WebAPI Web API docs label Apr 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 12, 2023

Preview URLs

Flaws (1)

Note! 1 document with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/Headers/getSetCookie
Title: Headers: getSetCookie() method
Flaw count: 1

  • bad_bcd_queries:
    • No BCD data for query: api.Headers.getSetCookie
External URLs (1)

URL: /en-US/docs/Web/API/Headers/getSetCookie
Title: Headers: getSetCookie() method

(comment last updated: 2023-04-17 17:10:45)

@Josh-Cena
Copy link
Member

Josh-Cena commented Apr 12, 2023

This sounds contradictory to the purpose of getSetCookie() — has this note interpreted the spec correctly, and if so, should I update it to list getSetCookie() as an exception to this rule?

AFAICT, this method is intended for server environments, such as Cloudflare workers and Node.js. Client code cannot create or obtain a response with Set-Cookie, but that's not fundamentally contradictory to this method's presence on an existing response. See: whatwg/fetch#973

@ricea
Copy link

ricea commented Apr 13, 2023

Because the Set-Cookie header is stripped from responses on the network, the example as it stands is misleading. I haven't been able to come up with a compelling example, but you can do something like:

const headers = new Headers({
    "Set-Cookie": "name=value",
});
headers.getSetCookie();

@chrisdavidmills
Copy link
Contributor Author

This sounds contradictory to the purpose of getSetCookie() — has this note interpreted the spec correctly, and if so, should I update it to list getSetCookie() as an exception to this rule?

AFAICT, this method is intended for server environments, such as Cloudflare workers and Node.js. Client code cannot create or obtain a response with Set-Cookie, but that's not fundamentally contradictory to this method's presence on an existing response. See: whatwg/fetch#973

Thanks @Josh-Cena , that makes total sense.

@chrisdavidmills
Copy link
Contributor Author

Because the Set-Cookie header is stripped from responses on the network, the example as it stands is misleading. I haven't been able to come up with a compelling example, but you can do something like:

Thanks @ricea. I've updated the page now; let me know what you think

Copy link

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm with nitpick

});
```

However, something like the following can be used to query `Set-Cookie` values on the server:
Copy link

Choose a reason for hiding this comment

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

This will work in the browser too, so you can remove the words "on the server".

It's only when communicating with the network that the Set-Cookie header is removed. As long as you stay in JavaScript, you can use it (it's just not very useful).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've updated the wording again but tried something slightly different to get all the points across that you made in the above message.

});
```

However, something like the following can be used to query `Set-Cookie` values. This is much more useful on the server, but it would also work on the client.
Copy link
Member

Choose a reason for hiding this comment

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

Another random drive-by: the motivation for the method is to retrieve multiple Set-Cookie values. Would be cool to have an example like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another random drive-by: the motivation for the method is to retrieve multiple Set-Cookie values. Would be cool to have an example like that.

Makes sense; I've updated the second example to show this.

Comment on lines 45 to 50
const headers = new Headers({
"Set-Cookie": "name=value",
"Set-Cookie": "name1=value1",
"Set-Cookie": "name2=value2",
});

headers.getSetCookie();
Copy link
Member

Choose a reason for hiding this comment

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

This is against how JS object literals work: later properties always override the former, so the Headers constructor cannot possibly see both properties. You need to set the header later.

const headers = new Headers({
  "Set-Cookie": "name1=value1",
});
headers.append("Set-Cookie", "name2=value2");
headers.getSetCookie();

(Can't test because my Chrome Canary doesn't seem to have this yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Darnit; that was a silly mistake. This is what happens when you try to do things quickly. Fixed now; thanks ;-)

And I tested the code on my Canary just to make sure.

Copy link

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@chrisdavidmills
Copy link
Contributor Author

lgtm, thanks!

@ricea super, thanks for the review.

@Elchi3 , I know you merged the BCD for this one. Can you give the method page a quick look and merge? Thanks in advance.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks Chris and everyone! 👍

@Elchi3 Elchi3 merged commit f334275 into mdn:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document Fetch's Headers's getSetCookie() method
4 participants