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

fix: support cloudflare workers #559

Merged
merged 3 commits into from
Feb 25, 2024
Merged

Conversation

riderx
Copy link
Contributor

@riderx riderx commented Feb 22, 2024

This PR comes to fix the Cloudflare workers issue mentioned here with the solution:
#366 (comment)

Fixes #366

@sindresorhus
Copy link
Owner

CI is failing

@riderx
Copy link
Contributor Author

riderx commented Feb 24, 2024

@sindresorhus fixed :)

this._options = {
// TODO: credentials can be removed when the spec change is implemented in all browsers. Context: https://www.chromestatus.com/feature/4539473312350208
credentials: (this._input as Request).credentials || 'same-origin',
credentials: isCredentialsSupported ? ((this._input as Request).credentials || 'same-origin') : 'same-origin',
Copy link

@fregante fregante Feb 24, 2024

Choose a reason for hiding this comment

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

I don’t think this is gonna work, _options.credentials should not be set at all, according to the error.

Also the comment is very outdated and we can probably use:

Suggested change
credentials: isCredentialsSupported ? ((this._input as Request).credentials || 'same-origin') : 'same-origin',
credentials: isCredentialsSupported ? (this._input as Request).credentials : undefined,

… as long as CloudFlare accepts {credentials: undefined} and doesn't actually require {}

by the way this would fix two duplicates:

@riderx
Copy link
Contributor Author

riderx commented Feb 24, 2024

Thanks @fregante i did update it.
I had to change the typing to make this solution work, and added a comment.
I did try and {credentials: undefined} is the required way for Cloudflare.
Since isCredentialsSupported is not reused, I wonder if that shouldn't be better, directly in the ternary?

@sindresorhus sindresorhus merged commit d8ee602 into sindresorhus:main Feb 25, 2024
1 check passed
alecmev added a commit to alecmev/ky that referenced this pull request Mar 21, 2024
alecmev added a commit to alecmev/ky that referenced this pull request Mar 21, 2024
alecmev added a commit to alecmev/ky that referenced this pull request Mar 22, 2024
sindresorhus#559 undid the fix in
sindresorhus#543. Added
`exactOptionalPropertyTypes` to `compilerOptions` to prevent this from
happening again, seems pretty low-impact. It should be okay to pass
`undefined` to `credentials` and other options, but this is an upstream
problem (see `RequestInit` in `undici-types/fetch.d.ts`).

The build was failing on TS 5.4 because there's a `priority` in
`RequestInit` now.
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.

The credentials field on RequestInitializerDict is not implemented; Cloudflare Workers issue.
3 participants