-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: cache mechanism for request with different headers #8754
fix: cache mechanism for request with different headers #8754
Conversation
🦋 Changeset detectedLatest commit: a885a67 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 |
You would need to update Also, I think we could probably have just a single hash computed from a combination of the headers and body |
Thanks! Note that const headers = new Headers();
headers.append('Authorization', `Bearer ${token}`);
fetch('/blah', { headers }); ...the hash will be all wrong because if (opts?.headers) {
- const headers = JSON.stringify(opts.headers);
+ const headers = [...new Headers(opts.headers)].join(',');
selector += `[data-headers-hash="${hash(headers)}"]`;
} |
@@ -262,10 +262,10 @@ test.describe('Load', () => { | |||
const payload_b = '{"status":200,"statusText":"","headers":{},"body":"Y"}'; | |||
// by the time JS has run, hydration will have nuked these scripts | |||
const script_contents_a = await page.innerHTML( | |||
'script[data-sveltekit-fetched][data-url="/load/serialization-post.json"][data-hash="3t25"]' | |||
'script[data-sveltekit-fetched][data-url="/load/serialization-post.json"][data-hash="4jrjph"]' |
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.
I still didn't figure out why this test is failing. I updated the hash to consider headers but it seems that the hash now it's different than the test above so maybe there is some header that I'm not considering when I hash manually.
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.
I found the issue, I was concatenating an ArrayBufferView + string.
To achieve this single hash computation I will need to update the hash function, I don't know if you want to do that. The other option is to concatenate the two hashes hash(headers) + hash(body)
thank you! |
- support caching of responses with `Vary` header (possible without any changes on the client because since #8754 we're taking headers into account for the cache key) - fix browser caching of adjacent pages/endpoints fixes #9780 --------- Co-authored-by: S. Elliott Johnson <[email protected]>
closes #8752
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:
.