fetch: Add Cloudflare Workers support (cross-fetch v4) #794
Conversation
|
|
|
I'll take a look at this--I'm not really familiar with how the CORS stuff is currently implemented in the SDK. The CI is failing on unrelated things which will be fixed once we merge #793 I'll rerun once I merge that. |
|
ok thanks ! yes, let me know what y'all think about removing that cors arg + that merge. |
There was a problem hiding this comment.
Ok, I don't have a problem with this as is. We'll have to keep an eye on cross-fetch to see when we can just swap to v4 stable.
For the cors stuff it looks like it was committed when we swapped from superagent to fetch, but the fetch library doesn't provide support for setting the mode property in requests, so it doesn't do anything.
Eric-Warehime
changed the title
|
While the |
Agreed!
The node-fetch library which is being folyfilled by cross-fetch doesn't ever use the Is there some place you know of that I'm not finding w/r/t the creation of the request object in browsers? |
The polyfills you mentioned should only ever be used in node. Our expectation is that the browsers we support are modern enough to supply their own native implementation of fetch -- no polyfill should be active for them. That's the environment that I think your analysis is missing. In this case, the best way to test that environment is to access a webpage from a real host (i.e. not localhost or accessed as a local file) using the SDK, and having code which talks to an algod located in a different host. That's the situation in which CORS would apply. |
I guess that includes react-native as well--I know we don't support it right now. Good point about the browsers. I'm unsure if our current browser tests will exercise cross origin support or not |
|
Hi, there is not really a need to polyfill fetch in 2023. It causes way more problems than it fixes across environments. Just use native fetch calls and the standard API and let us polyfill fetch in user space in the select places that is needed. Have a look at https://www.builder.io/blog/stop-polyfilling-fetch-in-your-npm-package |
|
Also, FWIW mode: "cors" is the default in the fetch api, you can safely omit it! https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch |
|
@spencercap The CI should pass if you rebase? Also I'm happy to merge this as long as @jasonpaulos doesn't object. |
spencercap
force-pushed
the
algosdk-plus-cross-fetch-4
branch
from
4d71e3f
to
0f7f83c
Compare
|
It looks like the new CI failures are because we're bumping the golang version everywhere, but we haven't updated sandbox yet. I've got algorand/sandbox#189 open to fix that and will re-run the CI once it's merged. |
|
any estimate when this will be pushed to the @latest release? |
* using new cross-fetch 4 w workers shim * removed mode: 'cors' for v8 compat
at the @thencc we're using the js algosdk in cloudflare workers' v8 environment for EXTREME server-side benefits.
by simply updating to
cross-fetchv4 (currently in alpha),algosdkcan work in chrome v8 in addition to the usual node + browser envs. this was just recently made available as discussed here: lquixada/cross-fetch#163as a byproduct, this PR removes the
mode: "cors"arg inalgosdk's fetch calls as it is not supported by the fetch api available within workers. may i ask what is the real use for this arg anyway?