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: revert using server read for load function fetch #12876

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Oct 25, 2024

closes #12868

This PR reverts a part of the change from #12113. The issue is caused by the read implementation trying to read the static asset from the server during a fetch, but it doesn't work since the static asset isn't packaged with the serverless function. I think having the distinction between fetch and read is good here.

} else if (read_implementation) {
const length = manifest._.server_assets[file];
const type = manifest.mimeTypes[file.slice(file.lastIndexOf('.'))];
return new Response(read_implementation(file), {
headers: {
'Content-Length': '' + length,
'Content-Type': type
}
});
}


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Oct 25, 2024

🦋 Changeset detected

Latest commit: 9a2fe16

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@eltigerchino
Copy link
Member Author

not sure why lint is failing on src/exports/vite/public.d.ts again. Formatting the file locally reveals no changes.

@eltigerchino eltigerchino added the pkg:adapter-vercel Pertaining to the Vercel adapter label Oct 25, 2024
aloisklink added a commit to aloisklink/sveltekit-2.7.0-issue-reproduction that referenced this pull request Oct 25, 2024
Copy link
Contributor

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this 8cba744 with aloisklink/sveltekit-2.7.0-issue-reproduction@2acc884 and it seems to work with Vercel serverless functions! See https://sveltekit-2-7-0-issue-reproduction-dp5l83ne5.vercel.app/

Be aware that this will probably cause a performance regression for Cloudflare/Node to pre-#12113.


I've also tested an alternative fix: aloisklink@f4aace9, which is just adding:

index 7e2177d66..417b59c93 100644
--- a/packages/kit/src/runtime/server/fetch.js
+++ b/packages/kit/src/runtime/server/fetch.js
@@ -97,7 +97,7 @@ export function create_fetch({ event, options, manifest, state, get_cookie_heade
 						return new Response(state.read(file), {
 							headers: type ? { 'content-type': type } : {}
 						});
-					} else if (read_implementation) {
+					} else if (read_implementation && file in manifest._.server_assets) {
 						const length = manifest._.server_assets[file];
 						const type = manifest.mimeTypes[file.slice(file.lastIndexOf('.'))];

According to aloisklink/sveltekit-2.7.0-issue-reproduction@c3ee13c and the Vercel preview at https://sveltekit-2-7-0-issue-reproduction-itnyzmr86.vercel.app/, it also seems to work, but I don't think it would cause the same performance regression on Cloudflare. But I unfortunately don't have a Cloudflare setup to test this!

@aloisklink
Copy link
Contributor

aloisklink commented Oct 25, 2024

not sure why lint is failing on src/exports/vite/public.d.ts again. Formatting the file locally reveals no changes.

Btw, I ran npx prettier --write src/exports/public.d.ts and made a PR that should fix this in #12877 Edit: it looks like the actual fix would be a bit more complicated, since we have conflicting lint rules, see #12877 (comment). Either way, this file is unrelated to the PR.

dummdidumm
dummdidumm previously approved these changes Oct 25, 2024
@dummdidumm dummdidumm dismissed their stale review October 25, 2024 09:36

better approach possible

@dummdidumm
Copy link
Member

I think that's right - we can actually just check if file is part of the manifest

@eltigerchino eltigerchino merged commit 4cdbf76 into main Oct 25, 2024
9 of 10 checks passed
@eltigerchino eltigerchino deleted the revert-server-fetch-read branch October 25, 2024 10:00
@github-actions github-actions bot mentioned this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:adapter-vercel Pertaining to the Vercel adapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

server fetch for relative asset paths in Vercel are broken since @sveltejs/kit 2.7.0
3 participants