Add avoidCors API to EdgeFetchFunction
#630
Conversation
samholmes
force-pushed
the
sam/fetchCorsForced
branch
from
fa6f1f7 to
e3582dc
Compare
There was a problem hiding this comment.
In the commit message, you say "idempotent" when you really mean "equivalent". The word "idempotent" means once you've done something once, doing it more times has no effect, like applying the Math.abs function.
Also, please edit the loginFetch function to use avoidCors: 'never'.
Also, in root.ts, find the makeSyncClient call and do fetch: (url, opts) => io.fetch(url, {...opts, avoidCors: 'never'}), to keep the behavior the same.
src/io/browser/browser-io.ts
Outdated
| if (avoidCors === 'never') { | ||
| return await fetchCorsProxy(uri, opts) | ||
| } | ||
| if (avoidCors === 'always') { | ||
| return await window.fetch(uri, opts) | ||
| } |
There was a problem hiding this comment.
I think these two conditions are backwards.
There was a problem hiding this comment.
If we are "never avoiding cross-origin request sharing" then we are "allowing cross-origin request sharing" which means we are allowing sharing between our origin (null) and the server's, so we should not let CORS policies block.
If we're are "always avoiding cross-origin request sharing" then we are "never cross-origin request sharing" which means if the server doesn't share our origin (null) then it should fail.
This is the logic I understood it as. Which is why it seemed a bit confusing to read and may not be obvious to the user if you're deriving an opposite conclusion from what I did when reading the API.
There was a problem hiding this comment.
Went with the "bypass" name instead, to make it clear what is meant.
| if (avoidCors === 'never') { | ||
| return await nativeFetch(uri, opts) | ||
| } | ||
| if (avoidCors === 'always') { | ||
| return await window.fetch(uri, opts) | ||
| } |
There was a problem hiding this comment.
I think these are backwards too. In order to "avoid" / dodge / bypass / escape the CORS restrictions that are blocking our request, we need to use the native fetch function. Hence, avoidCors: 'always' means "always use the native fetch function, and don't even bother with the web fetch."
src/types/types.ts
Outdated
| @@ -76,6 +76,8 @@ export interface EdgeIo { | |||
| /** | |||
| * This is like `fetch`, but will try to avoid CORS limitations | |||
| * on platforms where that may be a problem. | |||
| * | |||
| * @deprecated - Use `fetch` instead with `avoidCors: 'always'` option. | |||
There was a problem hiding this comment.
Actually avoidCors: 'auto' would be the correct equivalent. We could simplify the ask to /** @deprecated Use EdgeIo.fetch instead, which now includes CORS avoidance by default */, since the user doesn't need to do anything extra.
samholmes
force-pushed
the
sam/fetchCorsForced
branch
2 times, most recently
from
48f663f to
d1c8a97
Compare
This is to be able to to access `nativeFetch` in React Native environments. This parameter consolidates all fetch modes into one method. Setting `auto` (default) will act like old `fetchCors` method. Setting `never` will act like old `fetch` method. Setting `always` will forcibly use the CORS-safe fetching implementation (cors-servers in browser environments and `nativeFetch` in React Native environments). In Node environments, `never`, `auto` and `always` are all idempotent.
samholmes
force-pushed
the
sam/fetchCorsForced
branch
from
f26ada5 to
afe1a96
Compare
This is to be able to to access
nativeFetchin React Nativeenvironements. This
fetchCorsForcedmethod will also forcibly use theCORS servers in browser environemnts. It's idompodent in Node
environements.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none