-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 window.fetch
in load
functions to allow libraries to patch it
#10009
Conversation
🦋 Changeset detectedLatest commit: b1420c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
const patchedOpts = DEV | ||
? { | ||
...opts, | ||
__sveltekit_fetch__: true | ||
} | ||
: opts; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, this isn't super nice but it allows us to let the patched window.fetch
know that it was called via event.fetch
, therefore allowing us to always call window.fetch
.
window.fetch
instead of unpatched fetch in load
functionswindow.fetch
instead of unpatchable fetch in load
functions
@benmccann thanks for the review and sorry for missing out on the code styling. I made an additional small change (see comment above). Lmk If I can do anything else to help getting this through :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
native_fetch
still exists/is used in a few other places - is that deliberate?
I think it's still required for our own patches to |
Just wanted to say, big +1 to this PR, we could really use this in our app! |
window.fetch
instead of unpatchable fetch in load
functionswindow.fetch
in load
functions to allow libraries to patch it
This PR makes
initial_fetch
andsubsequent_fetch
use the patchedwindow.fetch
function instead of the unpatched and stored awaynative_fetch
function.This change brings one big benefit: Additional behaviour added by libraries (like Sentry but there are many others) that patch
window.fetch
is now also invoked inevent.fetch
calls within client-side universalload
functions.Additionally, this provides a way for libraries to instrument fetch in load functions without causing unintended cache misses (see getsentry/sentry-javascript#8174)
Kit patches
window.fetch
for two reasons:window.fetch
it inDEV
mode (this we should leave as-is)I'm not sure if I'm missing something but cleaning the cache should be alright in
load
functions as well 🤔Getting this PR merged would de-prioritize needing a dedicated
handleFetch
hook on the client-side (#9530) for us (which might still be worthwhile but for other reasons).Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.ref: #9530
ref: getsentry/sentry-javascript#8174
(ref: https://github.com/Lms24/gh-js-8174)