fix: make base URL for relative redirects consistent
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 testand lint the project withpnpm lintandpnpm check
Changesets
- [ ] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.
⚠️ 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
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.
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?
Just checking in - any updates on the issue? It sounds like the fix is quite complicated and the PR is not ready yet.
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.overlayto false resolves the flakiness in/test/apps/basics/ - current behavior before this PR depends on
get_navigation_result_from_branchmutating itsurlargument - function call graph for
src/runtime/client/client.jsis a bit complex: