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

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

mastermakrela
Copy link
Contributor

@mastermakrela mastermakrela commented Apr 12, 2024

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: e01efa1

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 Patch
@sveltejs/adapter-cloudflare Patch
@sveltejs/adapter-netlify Patch
@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 eltigerchino added the bug Something isn't working label Oct 10, 2024
@mastermakrela
Copy link
Contributor Author

@eltigerchino thank you for the review :)

I've corrected all the issues you've noted and tested the Cloudflare adapter again.

I see some tests in CI have failed, but as far as I understand, two of them should fail (Vercel ones)
and the third one (/prerendering/mutative-endpoint) does not seam to be connected with my changes.

Is there anything else I could/should do?

@eltigerchino
Copy link
Member

eltigerchino commented Oct 11, 2024

Thank you! This is a really good contribution. Don't worry about some of the tests which are flaky and pass when re-run.

cc: @dummdidumm do you mind taking another look at this?

@eltigerchino eltigerchino requested review from benmccann and removed request for benmccann October 11, 2024 01:39
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Fantastic work, and great PR description, was really easy to review because of that 👍

@dummdidumm dummdidumm merged commit df48fc6 into sveltejs:main Oct 11, 2024
12 checks passed
@github-actions github-actions bot mentioned this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't fetch assets using Kits fetch and relative path
3 participants