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 redirect logic to be able to handle dynamic parts in target paths #11184

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Joelkang
Copy link

@Joelkang Joelkang commented Jun 4, 2024

Context

There are a few related changes in this PR, but this was the use case I was trying to fix:
[category]/page/[page].astro is the route handler for myCategory/page/1.

I'm trying to redirect from /[category] to /[category]/page/1. Right now, since there's no RouteData with key /[category]/page/1 in 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 _redirect to be invalid. The line that gets generated is /:category 301

There 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

  1. Adds validation for the target such that target params should also be in source params, as per the current docs
  2. Resolve partially static redirect target paths to their route handler during manifest creation
  3. Fix underscore-redirect to generate the right _redirect for targets with dynamic parts

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

Copy link

changeset-bot bot commented Jun 4, 2024

⚠️ No Changeset found

Latest commit: 9cca1ad

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.

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jun 4, 2024
@@ -29,4 +29,282 @@ describe('Astro', () => {

assert.equal(_redirects.definitions.length, 2);
});

describe('Supports dynamic routes', () => {
Copy link
Author

@Joelkang Joelkang Jun 4, 2024

Choose a reason for hiding this comment

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

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.

const pattern =
'/' +
route.segments
.map(([part]) => {
//(part.dynamic ? '*' : part.content)
if (part.dynamic) {
if (part.spread) {
return '*';
return forTarget ? ':splat': '*';
Copy link
Author

Choose a reason for hiding this comment

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

.find(
(route) =>
route.pattern.test(destination) ||
route.fallbackRoutes.some((fallbackRoute) => fallbackRoute.pattern.test(destination))
Copy link
Author

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant