Fix redirect logic to be able to handle dynamic parts in target paths#11184
Fix redirect logic to be able to handle dynamic parts in target paths#11184Joelkang wants to merge 3 commits intowithastro:mainfrom
Conversation
|
github-actions
bot
added
the
pkg: astro
| assert.equal(_redirects.definitions.length, 2); | ||
| }); | ||
|
|
||
| describe('Supports dynamic routes', () => { |
There was a problem hiding this comment.
There's a lot of branching in the source file that isn't covered by these tests, but that's out of scope for my changes.
| if (part.dynamic) { | ||
| if (part.spread) { | ||
| return '*'; | ||
| return forTarget ? ':splat': '*'; |
There was a problem hiding this comment.
| .find( | ||
| (route) => | ||
| route.pattern.test(destination) || | ||
| route.fallbackRoutes.some((fallbackRoute) => fallbackRoute.pattern.test(destination)) |
There was a problem hiding this comment.
This is basically the same as matchRoute, but the current matchRoute code in match.js is for handling requests one level higher, and thus requires the entire Manifest
|
Are you still interested in working on this? |
|
@Princesseuh happy to, but I think I'd like more guidance on whether this is even worth doing. It's been a while since I've used astro (in part because of this bug), so I'll need to spend some time context switching back into it. Presently it's not clear to me whether this is even a problem worth solving / an actual bug, or whether its my poor interpretation of the docs. |
|
Looks like there's a bunch of conflict so it may be worth starting over to be honest, I'm sorry about that! I don't think it's a bug but rather a new feature. It doesn't need to go through the RFC process because it's fairly small tho. Thanks for trying and feel free to re-open a PR |
3 participants
Context
There are a few related changes in this PR, but this was the use case I was trying to fix:
[category]/page/[page].astrois the route handler formyCategory/page/1.I'm trying to redirect from
/[category]to/[category]/page/1. Right now, since there's no RouteData with key/[category]/page/1in the manifest, my redirect target falls back to the server-rendered path, which in my case for cloudflare, is the empty string '', causing the entry in_redirectto be invalid. The line that gets generated is/:category 301There is however, a RouteData in the manifest that's able to handle this path, so it should resolve that route at the key
/[category]/page/[page].Changes
underscore-redirectto generate the right _redirect for targets with dynamic partsThis should allow this comment and hack to be removed
Questions
Are there edge cases that I'm missing? Its not clear to me if this is actually a backwards compatible change. It feels like my changes align with what the code is supposed to do, but this might break some folks?
Testing
Added test for each of the 3 cases above that did not exist before.
Docs
/cc @withastro/maintainers-docs Not sure if we need to add more docs around what is permissible in the redirect config, but this really threw me off guard that I couldn't do what I thought I was able to do.