ofetch
ofetch copied to clipboard
fix: use wrapper to allow patching global `fetch`
๐ 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.
https://github.com/nuxt/test-utils/issues/775 just mentioning this one here, thanks for taking this up!
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)
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.
- Not a fan as well, but I guess it's the only way for them to intercept fetch calls
- 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
createFetchwe persist thefetchvariable)
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:
- 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.
- I implemented MSW in a Nuxt plugin (server + client) and a Nitro plugin (just in case)
- Mocking an API call with
useFetchseems to work for client-only calls, but fails for server calls - In the
node-appfolder 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 toalso increateFetchwe persist thefetchvariable?
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.
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
Related https://github.com/nuxt/nuxt/discussions/24519