Can't fetch assets using Kits `fetch` and relative path

mastermakrela commented 7 months ago

Describe the bug

According to the documentation (here and here), kits fetch should be able to handle relative paths on the server.

This works in development, but not in production (I've tested Cloudflare and node adapters).

If I do this:

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

export const GET = async ({ fetch: sk_fetch }) => {
    const _asset = await sk_fetch(asset);

    // we always end up here
    if (!_asset.ok)  return error(404, "Not found");

    return _asset;
};

The response is always this exact 404 (as far as I can tell): https://github.com/sveltejs/kit/blob/df6641a701f8155d7023ab6a5b17ef39df31d730/packages/kit/src/runtime/server/respond.js#L118-L120

We end up here, because the server_fetch doesn't recognise this path as an asset. Following check seams to be the problem:

https://github.com/sveltejs/kit/blob/df6641a701f8155d7023ab6a5b17ef39df31d730/packages/kit/src/runtime/server/fetch.js#L84-L87

as it doesn't check manifest._.server_assets

I've created an exhaustive reproduction that shows differences between sk/global fetch and absolute/relative path here: https://github.com/mastermakrela/sveltekit-relative-assets-fetch-bug

Solutions considered

Use read
- works only for node
- currently can't be implemented for Cloudflare, because read is sync and Cloudflares env.ASSETS.fetch isn't

Change is_asset check in Kit to

const is_asset =
    manifest.assets.has(filename) ||
    Object.prototype.hasOwnProperty.call(manifest._.server_assets, filename);

Change is_static_asset check in Cloudflare adapter to:

is_static_asset =
    manifest.assets.has(filename) ||
    manifest.assets.has(filename + '/index.html') ||
    Object.prototype.hasOwnProperty.call(manifest._.server_assets, filename);

Use global fetch
- as shown in https://github.com/mastermakrela/sveltekit-relative-assets-fetch-bug the simplest solution is just to use global fetch and absolute path
- this isn't optimal because for each asset a whole request is needed (performance cost)
- and one has to construct the absolute URL from vites asset import

Semantically async read + Cloudflare implementation, would be most satisfying, but it's probably leas viable, because of breaking change.

But independent of read, not being able to fetch assets using relative path on server seams to be a bug, that should be fixed (or if not at least a warning in dev mode would be nice).

Reproduction

https://github.com/mastermakrela/sveltekit-relative-assets-fetch-bug

Logs

No response

System Info

System:
    OS: macOS 14.4.1
    CPU: (8) arm64 Apple M1
    Memory: 69.52 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.7.1 - /opt/homebrew/bin/node
    npm: 10.5.0 - /opt/homebrew/bin/npm
    pnpm: 8.15.6 - /opt/homebrew/bin/pnpm
    bun: 1.0.25 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 123.1.64.109
    Chrome: 123.0.6312.87
    Edge: 112.0.1722.54
    Safari: 17.4.1
  npmPackages:
    @sveltejs/adapter-auto: ^3.2.0 => 3.2.0 
    @sveltejs/adapter-cloudflare: ^4.2.0 => 4.2.0 
    @sveltejs/adapter-node: ^5.0.1 => 5.0.1 
    @sveltejs/vite-plugin-svelte: ^3.0.2 => 3.0.2 
    svelte: 5.0.0-next.90 => 5.0.0-next.90 
    vite: ^5.2.7 => 5.2.7

Severity

serious, but I can work around it

Additional Information

No response

Rishab49 commented 7 months ago

I Think sk_fetch is causing the problem you should do this instead

export const GET: RequestHandler = async ({ fetch , url, platform }) => {

It works for me

mastermakrela commented 7 months ago

@Rishab49 sk_fetch is just a rename, the code above is equivalent to

export const GET = async (event) => {
    const sk_fetch = event.fetch;
    ...
}

I renamed it to clearly differentiate global fetch and the one from SvelteKit here

It works for me

Did you test it with Cloudflare adapter & wrangler? If yes, could you share a minimal setup? That would really help me.

Rishab49 commented 7 months ago

oh I forgot to test it with wrangler(apologies), its a bug

sveltejs / kit