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.

Sign up for GitHub

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

Jump to bottom

Document request options in one place #34437

Merged
merged 11 commits into from
Jul 2, 2024
Merged

Document request options in one place #34437

merged 11 commits into from
Jul 2, 2024

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Jun 26, 2024 •
edited
Loading

Per discussion in https://github.com/orgs/mdn/discussions/694.

I've been through the fetch() and Request() pages, incorporating what seemed useful, correcting what seem to be some errors[1] and adding some more detail. Also being more consistent about describing default values.

I think in some ways this goes alongside #34278, in that it would be nice to point to a usage guide for some of these options.

[1] e.g. that no-referrer passed into referrer works to omit the Referer header, that seems to be a confusion with referrerPolicy, or that "the Origin header is not set on Fetch requests with a method of HEAD or GET", which only seems to be true for same-origin requests.

Fixes #33356.
Fixes #13063.
@wbamberg wbamberg requested a review from a team as a code owner June 26, 2024 21:24
@wbamberg wbamberg requested review from sideshowbarker and removed request for a team June 26, 2024 21:24
@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Jun 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 26, 2024 •
edited
Loading

Preview URLs

(comment last updated: 2024-07-02 14:53:20)
@wbamberg wbamberg requested a review from a team as a code owner June 26, 2024 21:40
@wbamberg wbamberg requested review from pepelsbey and removed request for a team June 26, 2024 21:40
Josh-Cena
Josh-Cena reviewed Jun 26, 2024
View reviewed changes
files/en-us/web/api/requestinit/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/requestinit/index.md Outdated Show resolved Hide resolved
@wbamberg @Josh-Cena
Update files/en-us/web/api/requestinit/index.md
b592bef 
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
@sideshowbarker sideshowbarker removed their request for review June 27, 2024 08:39
@wbamberg
Merge remote-tracking branch 'upstream/main' into requestinit
460ca9a 
* upstream/main: (58 commits)
  Update arrow function documentation to clarify naming and assignment (mdn#34501)
  update fetch guide (mdn#34278)
  Replace alert in Learn/JavaScript/First_steps/Variables (mdn#34487)
  Replace alert in MDN/Writing_guidelines/Page_structures/Live_samples (mdn#34479)
  Fix typo (mdn#34486)
  Remove SVG color-profile attribute (mdn#34482)
  Remove SVG enable-background attribute (mdn#34483)
  Remove SVG kerning attribute (mdn#34475)
  Updated the description of `targetOrigin`  to specify the intended re… (mdn#34114)
  Mention CSWH in WebSocket server guide (mdn#34411)
  Add note to CSP sandbox saying allow-top-navigation is redundant (mdn#34415)
  Mention navigator.languages may be truncated and Accept-Language may have fallback (mdn#34418)
  Remove IDB output "example", preferring live example (mdn#34464)
  Mention that pinch-zoom are also wheel events (mdn#34468)
  Mention that flex-basis is floored at min-content (mdn#34469)
  More content to Global object glossary (mdn#34471)
  Fix IDB cursor prev direction description (mdn#34463)
  Remove all line number references to inline code examples (mdn#34459)
  Remove link to notification example (mdn#34412)
  Replaces HTML entity glossary links/mentions with char reference (mdn#34391)
  ...
@wbamberg
Add cross-links to guide
3acb84d
@wbamberg
Merge remote-tracking branch 'origin/requestinit' into requestinit
a346a00 
* origin/requestinit:
  Update files/en-us/web/api/requestinit/index.md
@wbamberg
Fix issue 13063: clean up Request.credentials
0f79e72
@wbamberg
Copy link
Collaborator Author

wbamberg commented Jun 29, 2024 •
edited
Loading

I just pushed 0f79e72 which I think means we can close #13063 in this PR.

Issue #13063 has a couple of aspects:

  • the page on Access-Control-Allow-Credentials incorrectly said that credentials included "authorization headers". It turns out I fixed that back in De-XHRify HTTP: everything except CORS #31013 , so now the page says that they may be "cookies, TLS client certificates, or authentication headers containing a username and password"
  • the page on Request.credentials was unclear/incorrect about this too. Also unrelated to this bug it says things like "whether the user agent should send or receive cookies from the other domain in the case of cross-origin requests" when it's more than just cookies, and more than just cross-origin requests. So in 0f79e72 I've taken out a lot of that stuff and pointed instead to Including credentials in the Fetch guide, which should be correct. This is also in line with the suggestion in Fix issue 33356: Document integrity properly #34406 that we ought to have quite minimal docs for these Request properties, since they are just reflections of the option given in RequestInit.
@wbamberg wbamberg mentioned this pull request Jun 29, 2024
Closed
@wbamberg
Copy link
Collaborator Author

wbamberg commented Jul 1, 2024

@sideshowbarker , I saw you removed my review request, I wasn't sure if that was because you didn't have time to or because @Josh-Cena was reviewing already. I'd still be very happy to get your review on it but I'm also happier about getting less detailed review since #34278 got merged :).
@sideshowbarker sideshowbarker self-requested a review July 2, 2024 01:00
@sideshowbarker
Copy link
Collaborator

@sideshowbarker , I saw you removed my review request, I wasn't sure if that was because you didn't have time to or because @Josh-Cena was reviewing already.

Yeah, that was the only reason — because it seemed like Josh was already on it.

I'd still be very happy to get your review on it but I'm also happier about getting less detailed review since #34278 got merged :).

I'll go ahead and do a review. It's always possible more eyes might catch something that otherwise for overlooked.
sideshowbarker
sideshowbarker reviewed Jul 2, 2024
View reviewed changes
Copy link
Collaborator

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

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

This all looks great to me — modulo the suggestions I made about the cache property
@wbamberg @sideshowbarker
Apply suggestions from code review
8563c90 
Co-authored-by: sideshowbarker <mike@w3.org>
@sideshowbarker sideshowbarker merged commit 413ce1e into mdn:main Jul 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
3 participants
@wbamberg @sideshowbarker @Josh-Cena