kit icon indicating copy to clipboard operation
kit copied to clipboard

fix: make base URL for relative redirects consistent

Open zqianem opened this issue 2 years ago • 3 comments

Will fix #9880. Only commit for now is adding to the tests from #8842; does this look like the behavior we want?

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • [x] 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:.

zqianem avatar May 10 '23 13:05 zqianem

⚠️ No Changeset found

Latest commit: 9c8703c5b774bc5d26433cac37a378e8cba42bf6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar May 10 '23 13:05 changeset-bot[bot]

Ideally yeah, I think this would be the behaviour. I looked into the implementation though and I think it's going to be quite tricky — at present, the information for each 'node' of a route (which basically consists of the page options + the result of calling load) is bundled together, meaning we can't determine the correct trailingSlash value without also running the load function... which throws an error, preventing us from reading the value.

So we'd need to separate those two things in order to make it possible to normalize pathnames prior to redirection. If that causes client.js to increase in size then we'll have to consider how important we think it is to cover this edge case. Ideally, redirects should probably be root-relative so that this situation doesn't arise.

Rich-Harris avatar May 17 '23 16:05 Rich-Harris

Here's a somewhat hacky implementation that covers the most common case when trailingSlashes isn't explicitly configured, as in the #9880 repro. It does run into the limitation where it can't read the config from files where the redirect is thrown, but ends up passing the tests because those throw the redirect in with-redirect/layout.js.

Should we also update the the docs to recommend absolute paths for redirects?

zqianem avatar May 17 '23 18:05 zqianem

Just checking in - any updates on the issue? It sounds like the fix is quite complicated and the PR is not ready yet.

dummdidumm avatar Jul 04 '23 10:07 dummdidumm

Thank you for the reminder.

I have been rather stumped by this — managed to get it working for .js but not for .server.js routes; I'll push those commits now. I'd be okay with it if anyone else wants to pick up this PR from here.

Some miscellaneous notes:

  • setting server.hmr.overlay to false resolves the flakiness in /test/apps/basics/
  • current behavior before this PR depends on get_navigation_result_from_branch mutating its url argument
  • function call graph for src/runtime/client/client.js is a bit complex: image

zqianem avatar Jul 04 '23 13:07 zqianem