fix: make base URL for relative redirects consistent #9897
Conversation
|
zqianem
changed the title
zqianem
changed the title
|
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 So we'd need to separate those two things in order to make it possible to normalize pathnames prior to redirection. If that causes |
|
Here's a somewhat hacky implementation that covers the most common case when Should we also update the the docs to recommend absolute paths for redirects? |
| /** | ||
| * @param {URL} url | ||
| * @param {Array<import('./types').BranchNode | undefined>} branch | ||
| */ | ||
| function normalize_url(url, branch) { | ||
| /** @type {import('types').TrailingSlash} */ | ||
| let slash = 'never'; | ||
| for (const node of branch) { | ||
| if (node?.slash !== undefined) slash = node.slash; | ||
| } | ||
| url.pathname = normalize_path(url.pathname, slash); | ||
| url.search = url.search; // turn `/?` into `/` | ||
| } |
There was a problem hiding this comment.
can we avoid mutating the URL here and instead return a new one? it's probably safe, but i've lost many hours of my life to these sorts of sneaky mutations in the past...
There was a problem hiding this comment.
Oh strange, it looks like some existing behavior (test: Routing › redirects from /routing/? to /routing ) actually depends on the mutation happening. Will look into it.
There was a problem hiding this comment.
Sorry for the delay. I've reverted the commit that makes this function immutable in case anyone wants to use this branch for a temporary fix — will continue looking into getting a per-branch config without running the load function.
| // TODO get per-branch config without running load | ||
| // https://github.com/sveltejs/kit/pull/9897#issuecomment-1551723833 | ||
| /** @type {Array<import('./types').BranchNode | undefined>} */ | ||
| const temp_branch = (await Promise.allSettled(branch_promises)).map((s) => | ||
| s.status === 'fulfilled' ? s.value : undefined | ||
| ); | ||
| normalize_url(url, temp_branch); |
There was a problem hiding this comment.
we really need to resolve the TODO before we can merge this :)
|
Just checking in - any updates on the issue? It sounds like the fix is quite complicated and the PR is not ready yet. |
The "foo" route itself defaults to a value of "never" for trailingSlash
This reverts commit d0f944e.
|
Thank you for the reminder. I have been rather stumped by this — managed to get it working for Some miscellaneous notes:
|
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:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.