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(fetch): support custom credentials in Request #82

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

MichaelDeBoey
Copy link
Contributor

See @kettanaito's remix-run#21

Motivation

I'm adopting this polyfill as a part of a large API change in Mock Service Worker to adhere better to the Fetch API specification (mswjs/interceptors#292, part of mswjs/msw#1404). While doing so, I've spotted that whenever I construct a Request instance, its credentials are always set to same-origin, ignoring any custom credentials value I may supply in request init.

It's absolutely possible and allowed to construct a Request instance with any credentials the consumer needs.

Context: I'm constructing Request instance for an intercepted XMLHttpRequest as a part of the @mswjs/interceptors library. When dealing with XHR, it controls the credentials behavior with its withCredentials?: boolean flag. I must different request credentials values based on that flag to respect the specification and keep the consumer's code consistent.

Changes

  • Adds credentials to RequestState
  • Updates Request.credentials getter to resolve the value from the internal state.
  • Adds missing test suites for request credentials behaviors

CC/ @alanshaw @Gozala

Copy link
Contributor

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

👍 Thanks for forwarding this. Let's improve the spec compliance.

@MichaelDeBoey
Copy link
Contributor Author

MichaelDeBoey commented Aug 27, 2023

@kettanaito Yeah it would be nice to get everything that's in the @remix-run fork back upstream

@alanshaw @Gozala I'm quite certain that if you want to add maintainers to the project, some Remix people (like @jacob-ebey) would be happy to join! 👀

@kettanaito
Copy link
Contributor

@MichaelDeBoey, is there any particular reason Remix still needs these polyfills? Migrating to Next 18 and using the global Fetch API should suit all the needs, should it not?

@MichaelDeBoey
Copy link
Contributor Author

@kettanaito Fetch API is still marked Experimental in the Node docs, so the team decided to not rely on them (yet) until they're marked Stable

@Gozala Gozala merged commit 1da95bc into web-std:main Aug 28, 2023
@MichaelDeBoey MichaelDeBoey deleted the request-credentials branch August 29, 2023 00:22
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