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

feat: use config.kit.paths.base for static assets #4448

Merged
merged 13 commits into from
Oct 12, 2022

Conversation

hmnd
Copy link
Contributor

@hmnd hmnd commented Mar 24, 2022

Prefixes appDir with config.kit.paths.base for adapter-(cloudflare, cloudflare-workers, netlify). Didn't touch node, as there's some discussion around how that should be handled differently in #3726. Would be great if @asendia and @tv42 could test the adapters for their respective issues, since I've never used those platforms with sveltekit.

I'm aware it would be better if the base path was handled outside of SvelteKit instead of creating nested folders like this, but (at least for workers sites), I can't think of a better alternative.

fixes #4442, fixes #2843, fixes #3726

  • add manifest.prefix and builder.getAppPrefixDirectory()
  • adapter-cloudflare*: use manifest.prefix instead of manifest.appDir
  • adapter-netlify: use prefixed appDir for cache headers
  • adapter-vercel: use prefixed appDir for routes.json
  • adapter-cloudflare, adapter-cloudflare-workers, adapter-netlify: write static assets to "$dest/$base"

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Mar 24, 2022

🦋 Changeset detected

Latest commit: 069a17a

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

This PR includes changesets to release 7 packages
Name Type
@sveltejs/adapter-cloudflare Patch
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/adapter-node Patch
@sveltejs/adapter-vercel Patch
@sveltejs/kit Patch
@sveltejs/adapter-auto 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

@asendia
Copy link
Contributor

asendia commented Mar 25, 2022

Hi @hmnd, thank you for the quick action, I can confirm that the assets directory now includes the base path (path = admin):
image

In the build/_headers file, I see double slash //, is this expected?

//admin/_app//*
  cache-control: public
  cache-control: immutable
  cache-control: max-age=31536000

@hmnd
Copy link
Contributor Author

hmnd commented Mar 25, 2022

@asendia No problem. Wanted to fix this for the workers adapter, and then saw your issue pop up too.

Thanks for catching that! Should be fixed now.

@arggh
Copy link

arggh commented Mar 25, 2022

Thanks for this PR, perfect timing 👌 I gave it a try and the first deploy failed with...

17:21:23.048 | > Cannot find package 'devalue' imported from /opt/buildhome/repo/app/.svelte-kit/output/server/index.js
-- | --
17:21:23.049 | at new NodeError (node:internal/errors:371:5)
17:21:23.049 | at packageResolve (node:internal/modules/esm/resolve:932:9)
17:21:23.049 | at moduleResolve (node:internal/modules/esm/resolve:978:18)
17:21:23.049 | at defaultResolve (node:internal/modules/esm/resolve:1080:11)
17:21:23.049 | at ESMLoader.resolve (node:internal/modules/esm/loader:530:30)
17:21:23.049 | at ESMLoader.getModuleJob (node:internal/modules/esm/loader:251:18)
17:21:23.049 | at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:79:40)
17:21:23.049 | at link (node:internal/modules/esm/module_job:78:36)

I'm not sure whether it failed due to a misconfiguration on my part while running the forked version of SvelteKit, or the actual changes in this PR.

I got it fixed by adding devalue as a dependency to my app, and can confirm that the kit.paths.base configuration is now working on Cloudflare as expected 👍

@hmnd
Copy link
Contributor Author

hmnd commented Mar 28, 2022

@asendia @arggh FYI in case you need this to work asap like me, I've published the fork at @icewolf/svelte-kit@1.0.0-next.303.bleeding.0 while this is awaiting review.
pnpm add -D "@sveltejs/kit@npm:@icewolf/svelte-kit@^1.0.0-next.303.bleeding"

@dmkret
Copy link

dmkret commented Apr 4, 2022

Hi, @hmnd! Can you add the fix to adapter-node too?

@arggh
Copy link

arggh commented Apr 24, 2022

@hmnd not sure if I'm doing something funky, but for some reason files in /static aren't being found when using kit.paths.base. I just get the SvelteKit 404 page. Imported assets work.

@benmccann
Copy link
Member

@hmnd thanks for this and sorry for letting it sit! It looks like it will need to be rebased since #5618 (removes writeStatic) and #5332 (moves core/dev and code/build to vite folder) were merged

@benmccann benmccann added p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. paths.base bugs relating to `config.kit.paths.base` labels Jul 20, 2022
@hmnd hmnd force-pushed the fix/cf-basepath-support branch 2 times, most recently from d449b20 to 238c07c Compare July 22, 2022 03:15
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

It looks like this missed a place where Netlify Edge and adapter-node need to be updated. I'll take a stab at that

@benmccann
Copy link
Member

benmccann commented Oct 12, 2022

I missed that the PR description said adapter-node was skipped on purpose and you preferred to handle that one separately. Anyway, I updated this PR to do the simple fix of just writing the files to a sub-directory with the base path for adapter-node in the same way as the others. That should be enough to get things working for 1.0

I created a new issue for the better fix of mounting the directories under a different path at runtime rather than changing the location on disk: #7242

Right now, writeClient and writePrerendered are always called with the base path as a suffix, so I initially I thought we might be able to refactor by moving that inside those methods, but that wouldn't work if we allow changing mount points in the future or have dynamic base paths.

This doesn't touch adapter-static because I didn't have time and couldn't find that anyone filed an issue for that one besides the dynamic base paths issue

@dummdidumm dummdidumm merged commit b4c2680 into sveltejs:master Oct 12, 2022
@hmnd hmnd deleted the fix/cf-basepath-support branch October 13, 2022 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. paths.base bugs relating to `config.kit.paths.base`
Projects
None yet
6 participants