Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use wrapper to allow patching global fetch #377

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dwightjack
Copy link

πŸ”— Linked issue

#295

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ 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

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

@rinux55
Copy link

rinux55 commented Mar 21, 2024

nuxt/test-utils#775 just mentioning this one here, thanks for taking this up!

@pi0 pi0 changed the title Refactor: store a local reference of fetch as a wrapper to allow patching fix: use wrapper to allow patching global fetch Mar 21, 2024
@pi0
Copy link
Member

pi0 commented Mar 21, 2024

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 pi0 marked this pull request as draft March 21, 2024 12:43
@dwightjack
Copy link
Author

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 (4026829).

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?

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

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

Project coverage is 69.80%. Comparing base (27996d3) to head (f8a071b).
Report is 9 commits behind head on main.

Files Patch % Lines
src/index.ts 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #377       +/-   ##
===========================================
+ Coverage   56.86%   69.80%   +12.93%     
===========================================
  Files          16       15        -1     
  Lines         728      606      -122     
  Branches      113      116        +3     
===========================================
+ Hits          414      423        +9     
+ Misses        303      173      -130     
+ Partials       11       10        -1     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@vanstinator
Copy link

vanstinator commented Mar 23, 2024

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!

evanderkoogh/otel-cf-workers#96

@MuhammadM1998
Copy link

Related nuxt/nuxt#24519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants