Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

replace cross-fetch with isomorphic-unfetch #25500

Closed
wants to merge 1 commit into from
Closed

replace cross-fetch with isomorphic-unfetch #25500

wants to merge 1 commit into from

Conversation

wentokay
Copy link
Contributor

@wentokay wentokay commented May 23, 2022

Problem

cross-fetch injects XMLHttpRequest into the bundled outputs of web3.js and this is incompatible with workers, so web3.js cannot run inside web or service workers.

This also means it can't run inside server-based workers like 'cloudflare workers'

As a result it might be incompatible with some of the suggested deployment paths for popular tools like https://remix.run

Finally it is also adding extra unnecessary bloat into browsers where fetch is already supported e.g. var xhr = new XMLHttpRequest(); in https://cdn.jsdelivr.net/npm/@solana/web3.js@1.43.1/lib/index.browser.esm.js

Summary of Changes

replace cross-fetch with isomorphic-unfetch ponyfill

Compatibility

node browser worker
fetch
cross-fetch (2.8kB)
isomorphic-unfetch (591B)

Tested as working for

  • node (commonjs)
  • node (esmodules)
  • browser <script type="text/javascript">
  • browser <script type="module">
  • inside a serviceworker

by running

yarn build && yarn link in this PR branch

then

yarn link @solana/web3.js & running each of the package.json scripts in https://github.com/wentokay/unfetch-test

related:

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #25500 (6a56e9e) into master (f9f6b94) will decrease coverage by 6.6%.
The diff coverage is n/a.

❗ Current head 6a56e9e differs from pull request most recent head cd34f4d. Consider uploading reports for the commit cd34f4d to get more accurate results

@@             Coverage Diff             @@
##           master   #25500       +/-   ##
===========================================
- Coverage    82.0%    75.3%     -6.7%     
===========================================
  Files         655       39      -616     
  Lines      171822     2337   -169485     
  Branches      335      335               
===========================================
- Hits       140972     1762   -139210     
+ Misses      30734      459    -30275     
  Partials      116      116               

@wentokay
Copy link
Contributor Author

I was unsure what to do with two lockfiles so I ran both yarn install and npm install although perhaps this is problematic?

@t-nelson
Copy link
Contributor

whoever reviews this, please be vigilant of supply chain attacks. we don't generally accept this type of change from external contributors

@steveluscher
Copy link
Contributor

We’re actually going to eliminate the fetch polyfill entirely and ask that folks using Node older than 17.5 to bring their own polyfill. See #25014.

@steveluscher
Copy link
Contributor

For instance, for Remix specifically: https://remix.run/docs/styling/v1/other-api/node

@steveluscher
Copy link
Contributor

Thanks for opening this PR though! Super good call out about service workers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants