-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: use window.fetch in load functions to allow libraries to patch it
#10009
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
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 :) |
dummdidumm
left a comment
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 ensures that this server-load data request is also patchable by basically using the same mechanism introduced in #10009
This PR makes
initial_fetchandsubsequent_fetchuse the patchedwindow.fetchfunction instead of the unpatched and stored awaynative_fetchfunction.This change brings one big benefit: Additional behaviour added by libraries (like Sentry but there are many others) that patch
window.fetchis now also invoked inevent.fetchcalls within client-side universalloadfunctions.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.fetchfor two reasons:window.fetchit inDEVmode (this we should leave as-is)I'm not sure if I'm missing something but cleaning the cache should be alright in
loadfunctions as well 🤔Getting this PR merged would de-prioritize needing a dedicated
handleFetchhook 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 testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand 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)