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: prevent incorrect trailing slash redirect for prerendered root page when paths.base is set #10763

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Sep 21, 2023

fixes #9982

This PR reinforces the behaviour of always redirecting /base to /base/ in #9343 by:

  • fixing the client-side trailing slash redirect incorrectly redirecting from /base/ to /base (because no trailing slash option is set).
  • when running vite preview and visiting the prerendered root page without the trailing slash /base, correctly redirect to the trailing slash path /base/.
  • keep search params intact for vite preview trailing slash redirect

Debating adding a test for this because it would require setting up another test project just to test the behaviour of prerendering a root page with paths.base set.

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

@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2023

🦋 Changeset detected

Latest commit: 6cd4623

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

@eltigerchino eltigerchino changed the title fix: redirect to trailing slash root page when paths.base is set fix: prevent incorrect trailing slash redirect on root page when paths.base is set Sep 21, 2023
@benmccann benmccann added the paths.base bugs relating to `config.kit.paths.base` label Sep 21, 2023
@eltigerchino eltigerchino changed the title fix: prevent incorrect trailing slash redirect on root page when paths.base is set fix: prevent incorrect trailing slash redirect for prerendered root page when paths.base is set Sep 23, 2023
@dummdidumm dummdidumm merged commit 5b1d1ab into master Dec 13, 2023
14 checks passed
@dummdidumm dummdidumm deleted the fix-prerender-with-base branch December 13, 2023 08:57
@github-actions github-actions bot mentioned this pull request Dec 13, 2023
const { pathname } = new URL(original_url, 'http://dummy');
const { pathname, search } = new URL(original_url, 'http://dummy');

// if `paths.base === '/a/b/c`, then the root route is `/a/b/c/`,
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an explanation of why that is? It seems quite surprising to me that we would ignore the trailing slash option for the root page when base is set. I remember putting a lot of effort into an earlier version of Vite to allow it to work without a trailing slash and I don't understand why we'd have this new behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

The original explanation is in this previous PR #9343 However it was not applied in vite preview, hence this PR. But I think this got removed in 2.0 because vite always does it? I have to check

Copy link
Member

Choose a reason for hiding this comment

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

Oops, sorry, I didn't mean to comment on a closed PR. I was on mobile and thought I was looking at #11357

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paths.base bugs relating to `config.kit.paths.base`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Path in svelte.config.js is not always respected, leading to it breaking
3 participants