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: handle relative assets paths in server fetch correctly #12113

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

Conversation

mastermakrela
Copy link

The Problem

Fixes #12081 (and maybe helps with #3850).

TL;DR:

// +server.ts
import asset from "$lib/image.png";

export const GET = async ({ fetch }) => { return fetch(asset); };

works in dev and preview, but always returns 404 with adapter-cloudflare and adapter-node.

More details and exhaustive reproduction in #12081 and here

The Solution

I've extended all places that check manifest.assets to also check manifest._.server_assets.
This has two benefits:

  1. the adapters can return the file directly without calling the SvelteKit server
  2. server_fetch (in packages/kit/src/runtime/server/fetch.js) can return the file earlier (previously it fell through to response)

This should always work because since #11649 all server assets should be copied to the output folder (source).

Other improvements

If read implementation is available, server_fetch will use it to get the files directly — should be theoretically faster than a fetch.

This PR is similar to what #3850 proposed, but because state.read/options.read expects a function that returns a buffer and read_implementation returns a stream, they aren't compatible. Also, it still wouldn't work with Cloudflare, because there the “file read” is an async fetch.

But this PR brings us as close as possible to #3850 as currently possible, because:

  • node will use read — skips overhead of fetch
  • Cloudflare will fetch static asset directly

and both of them (and maybe more adapters that I didn't test) will work with relative paths on the server.


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 Apr 12, 2024

🦋 Changeset detected

Latest commit: 1460fc7

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

This PR includes changesets to release 4 packages
Name Type
@sveltejs/adapter-cloudflare-workers Minor
@sveltejs/adapter-cloudflare Minor
@sveltejs/adapter-netlify Minor
@sveltejs/kit Minor

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

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.

Can't fetch assets using Kits fetch and relative path
1 participant