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

always add trailing slash to base path #9343

Merged
merged 3 commits into from
Apr 17, 2023
Merged

always add trailing slash to base path #9343

merged 3 commits into from
Apr 17, 2023

Conversation

Rich-Harris
Copy link
Member

fixes #9341.

In #9220, we changed the logic for resolving asset paths to accommodate the relative: true option, and included special handling for the case where you're rendering the root page when base is set — if the base path is /a/b/c and you render /a/b/c, then the relative path (somewhat counter-intuitively) needs to be ./c for links to work.

Turns out that was a mistake, because the root should always end with a trailing slash — if you prerender that page, it will be output as <output>/index.html, and relative paths will thus be incorrectly converted to <output>/c/....

The fix is to remove the special case handling, and ensure that /a/b/c always redirects to /a/b/c/. This is much simpler and more correct. It arguably falls into the semver gray zone where someone somewhere might be relying on the buggy behaviour, but I think it's vanishingly unlikely, and in any case the worst that will happen is a URL that used to render something gets turned into a redirect.

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

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.

Would an alternative solution of checking whether or not we're on the base path with a trailing slash work, too? If you're on /base/, do like this fix does, if you're on /base, keep the old behavior.

packages/kit/src/runtime/server/respond.js Show resolved Hide resolved
@Rich-Harris
Copy link
Member Author

If you're on /base/, do like this fix does, if you're on /base, keep the old behavior.

/base/ was already treated correctly, it was only /base where things (quite reasonably, in retrospect) got messed up

@mdi22
Copy link

mdi22 commented Mar 9, 2023

Dear @Rich-Harris,

I tried the PR with my code, now.
Unfortunately, this PR only partly solves the problem (at least for me).

It works fine when I call the base path directly, in my case "http://localhost/build"

It also works following links to routes inside the already loaded SPA.

It does not work when calling a route below the base path in the browser directly, in my case "http://localhost/build/Termine" or "http://localhost/build/Termine/" (with or without trailing slash)

Am I missing something?

I'll attach the created scipt tag in index.html in different versions:
URL "http://.../build/Termine"

Sveltekit version 1.8, working correctly:

<script>
    {
        __sveltekit_rs4xzv = {
            env: {},
            assets: "/build",
            element: document.currentScript.parentElement
        };

        Promise.all([
            import("/build/_app/immutable/entry/start.3791b6e2.js"),
            import("/build/_app/immutable/entry/app.e98f8777.js")
        ]).then(([kit, app]) => {
            kit.start(app, __sveltekit_rs4xzv.element);
        });
    }
</script>

Sveltekit version 1.11, neither working on base URL nor on path under base URL:

<script>
    {
        __sveltekit_1h6aion = {
            env: {},
            assets: "/build",
            base: new URL("./build", location).pathname,
            element: document.currentScript.parentElement
        };

        Promise.all([
            import("./build/_app/immutable/entry/start.6980a169.js"),
            import("./build/_app/immutable/entry/app.35650362.js")
        ]).then(([kit, app]) => {
            kit.start(app, __sveltekit_1h6aion.element);
        });
    }
</script>

Sveltekit version PR 9343, working correctly on base URL, not on route under base URL:

<script>
    {
        __sveltekit_inylau = {
            env: {},
            assets: "/build",
            base: new URL(".", location).pathname.slice(0, -1),
            element: document.currentScript.parentElement
        };

        Promise.all([
            import("./_app/immutable/entry/start.a989781b.js"),
            import("./_app/immutable/entry/app.3bd555c8.js")
        ]).then(([kit, app]) => {
            kit.start(app, __sveltekit_inylau.element);
        });
    }
</script>

My paths / base setting is '/build', but could be anything.

The problem seems to be that the path "./_app/immutable/entry/start.a989781b.js" is evaluated by the browser to
/build/Termine/_app/immutable/entry/start.a989781b.js

But only the file
/build/_app/immutable/entry/start.a989781b.js
was created

My .htaccess file is as follows

RewriteEngine On
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule .* index.html [NC,L]

I was not able to find a working RewriteRule to work around the new behavior.

Would it be the best to always use an absolute path when using paths/base?

Thank you already!

@Rich-Harris
Copy link
Member Author

It sounds like you're rewriting all paths to index.html? If so, there's no reason to expect that to work! You need to use a fallback page (which must use absolute paths) if you're building an SPA.

@mdi22
Copy link

mdi22 commented Apr 6, 2023

Dear @Rich-Harris

It sounds like you're rewriting all paths to index.html? If so, there's no reason to expect that to work! You need to use a fallback page (which must use absolute paths) if you're building an SPA.

thanks a lot, that was the rescuing hint!
It's all in the documentation (https://kit.svelte.dev/docs/single-page-apps), but somehow I missed that before, and it worked with the old Sveltekit.

I can confirm that using a "real" fallback page like documented everything is fine - with Sveltekit 1.15 and with the version of this branch.

Thanks again and Happy Easter!

@benmccann benmccann mentioned this pull request Apr 10, 2023
5 tasks
@changeset-bot
Copy link

changeset-bot bot commented Apr 11, 2023

🦋 Changeset detected

Latest commit: 445b2a8

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants