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] strip paths.base before resolving assets in preview server. #2409

Merged
merged 2 commits into from Sep 22, 2021

Conversation

Karlinator
Copy link
Contributor

@Karlinator Karlinator commented Sep 12, 2021

The preview server was implicitly assuming that asset URLs would never start with the base path. In fact, they always do.

Vaguely related to #2230 (but this was a new issue I discovered and diagnosed while working on #2407).

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 Sep 12, 2021

🦋 Changeset detected

Latest commit: 19c170b

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

@Karlinator Karlinator changed the title [fix] remove paths.base from all requests in preview mode. [fix] strip paths.base before resolving assets in preview server. Sep 12, 2021
@benmccann
Copy link
Member

@Karlinator can you rebase this PR?

@Karlinator
Copy link
Contributor Author

Sure thing!

Prettier is being weird for me right now. It doesn't agree with itself between the CI Lint and running lint or format locally, but I figured it out eventually.

@Karlinator
Copy link
Contributor Author

Karlinator commented Sep 20, 2021

I've noticed static assets are available both at /base-path/_app/whatever and at /_app/whatever. I can add a check to reject with 404 all requests that don't start with the base path, but looking at this I suspect there are more errors in the if/else stuff around 110? Or at the very least it's confusing as it is now. I'll have a little more of a think.

@benmccann
Copy link
Member

Hmm. Honestly, I'd love to just get rid of all this stuff and call Vite's preview functionality. It looks like their preview method is not exposed though, so that would be the first step

@Karlinator
Copy link
Contributor Author

I mean I don't disagree. But I think the base path stuff would still be a problem (we apparently have bugs in the dev server about this, see #2465 as you know). However, right now we are basically supporting two quite separate and different servers for preview and dev which isn't great.

@benmccann
Copy link
Member

Yeah, if there are issues in the dev server that's probably the more important one to fix since it's more used and will stick around longer. This maybe wouldn't be worth as much effort if the code were all about to be rewritten, but that would depend on vitejs/vite#5014 and I don't know how long that'd take or if it will even be accepted, so probably worth getting this in still

@benmccann benmccann merged commit fb2e134 into sveltejs:master Sep 22, 2021
@Karlinator
Copy link
Contributor Author

I won't spend any more time trying to improve/clean up this until we know the outcome of vitejs/vite#5014 then. Hopefully we can just rip this all out soon-ish.

@raduab
Copy link
Contributor

raduab commented Nov 25, 2021

I am getting the following error when building, starting with version 171:

> Using @sveltejs/adapter-node
 > .svelte-kit/output/server/chunks/app-0d4ff97d.js:1703:25: error: Could not resolve "./global-61357231.js"
    1703 │       load: () => import("./global-61357231.js")
         ╵                          ~~~~~~~~~~~~~~~~~~~~~~

 > .svelte-kit/output/server/chunks/__layout-048eba31.js:377:7: error: Could not resolve "./global-61357231.js"
    377 │ import("./global-61357231.js");
        ╵        ~~~~~~~~~~~~~~~~~~~~~~

> Build failed with 2 errors:
.svelte-kit/output/server/chunks/__layout-048eba31.js:377:7: error: Could not resolve "./global-61357231.js"
.svelte-kit/output/server/chunks/app-0d4ff97d.js:1703:25: error: Could not resolve "./global-61357231.js"
Error: Build failed with 2 errors:
.svelte-kit/output/server/chunks/__layout-048eba31.js:377:7: error: Could not resolve "./global-61357231.js"
.svelte-kit/output/server/chunks/app-0d4ff97d.js:1703:25: error: Could not resolve "./global-61357231.js"

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.

None yet

3 participants