ofetch icon indicating copy to clipboard operation
ofetch copied to clipboard

fix: use wrapper to allow patching global `fetch`

Open dwightjack opened this issue 1 year ago โ€ข 6 comments

๐Ÿ”— Linked issue

https://github.com/unjs/ofetch/issues/295

โ“ Type of change

  • [ ] ๐Ÿ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • [ ] ๐Ÿž Bug fix (a non-breaking change that fixes an issue)
  • [x] ๐Ÿ‘Œ Enhancement (improving an existing functionality like performance)
  • [ ] โœจ New feature (a non-breaking change that adds functionality)
  • [ ] ๐Ÿงน Chore (updates to the build process or auxiliary tools and libraries)
  • [ ] โš ๏ธ Breaking change (fix or feature that would cause existing functionality to change)

๐Ÿ“š Description

I was investigating the usage of MSW on Nuxt 3 and encountered #295. Looking at the source code, I think that a possible solution is to define the local fetch constant as getter instead of a reference to globalThils.fetch. In this way, 3rd party library can patch and overwrite the global object without us referencing an outdated function:

// Before
export const fetch = _globalThis.fetch

// After
export const fetch = (...args: Parameters<typeof globalThis.fetch>) => _globalThis.fetch(...args)

At the moment, I cannot think of possible breaking changes or side effects caused by this change, but I might miss something.

๐Ÿ“ Checklist

  • [x] I have linked an issue or discussion.
  • [ ] I have updated the documentation accordingly.

dwightjack avatar Mar 21 '24 05:03 dwightjack

https://github.com/nuxt/test-utils/issues/775 just mentioning this one here, thanks for taking this up!

rinux55 avatar Mar 21 '24 07:03 rinux55

Thanks for PR. I was thinking of same solution too. I agree it is needed but (sadly) means 1) we allow patching behavior of a standard primitive 2) adds small call overhead.

@dwightjack Have you tried this solution btw? I guess we might need to patch the node entrypoint as well. (also in createFetch we persist the fetch variable)

pi0 avatar Mar 21 '24 12:03 pi0

Thanks for PR. I was thinking of same solution too. I agree it is needed but (sadly) means 1) we allow patching behavior of a standard primitive 2) adds small call overhead.

  1. Not a fan as well, but I guess it's the only way for them to intercept fetch calls
  2. I am not sure how much this could impact the performances ๐Ÿค” For sure, it's an unneeded change for many use cases.

@dwightjack Have you tried this solution btw? I guess we might need to patch the node entrypoint as well. (also in createFetch we persist the fetch variable)

Oh, I missed that part. I updated the PR (https://github.com/unjs/ofetch/pull/377/commits/4026829a54b6d08275c01cb29524ecd7da708359).

I wasn't able to make a full reproduction of Nuxt + MSW on StackBlitz (I guess there are some problems with service workers), so I pushed it to a GitHub repository: https://github.com/dwightjack/ofetch-msw-reproduction

Some notes:

  1. The repository includes the packed version of ofetch built from this PR (I am not sure if there's a more elegant way to do this) and I defined an override for every instance of ofetch.
  2. I implemented MSW in a Nuxt plugin (server + client) and a Nitro plugin (just in case)
  3. Mocking an API call with useFetch seems to work for client-only calls, but fails for server calls
  4. In the node-app folder I ported the small node application linked in the original issue using both the un-modified and the patched versions of ofetch. In that case, the patched version seems to work correctly, so I am not sure why it is not working on Nuxt. ๐Ÿค” Could it be related to also in createFetchwe persist thefetch variable?

dwightjack avatar Mar 22 '24 03:03 dwightjack

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.08%. Comparing base (27996d3) to head (63afebd). Report is 35 commits behind head on main.

Files Patch % Lines
src/index.ts 0.00% 3 Missing :warning:
src/node.ts 66.66% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #377       +/-   ##
===========================================
+ Coverage   56.86%   67.08%   +10.22%     
===========================================
  Files          16       16               
  Lines         728      474      -254     
  Branches      113      116        +3     
===========================================
- Hits          414      318       -96     
+ Misses        303      146      -157     
+ Partials       11       10        -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 22 '24 09:03 codecov[bot]

I'm working to add some CF Pages support to otel-cf-workers and auto-instrumentation of fetch has no effect there for folks using ofetch, because by the time globalThis.fetch is patched by otel-cf-workers ofetch has already initialized and doesn't pick up on the new instrumented fetch object assigned to globalThis.fetch.

I think the work being done here would unblock this use-case too!

https://github.com/evanderkoogh/otel-cf-workers/issues/96

vanstinator avatar Mar 23 '24 13:03 vanstinator

Related https://github.com/nuxt/nuxt/discussions/24519

MuhammadM1998 avatar Apr 30 '24 17:04 MuhammadM1998