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

Experimental redirects: Dynamic routes interfere with redirects in SSR mode #7581

Closed
1 task
paulrudy opened this issue Jul 5, 2023 · 2 comments · Fixed by #7644
Closed
1 task

Experimental redirects: Dynamic routes interfere with redirects in SSR mode #7581

paulrudy opened this issue Jul 5, 2023 · 2 comments · Fixed by #7644
Assignees
Labels
needs response Issue needs response from OP

Comments

@paulrudy
Copy link
Contributor

paulrudy commented Jul 5, 2023

What version of astro are you using?

2.7.4

Are you using an SSR adapter? If so, which one?

Vercel

What package manager are you using?

pnpm

What operating system are you using?

Mac

What browser are you using?

Chrome, Safari

Describe the Bug

In SSR mode, I'm using a dynamic route [...slug] to serve pages from a content collection. I'm also trying to use experimental redirects to redirect specific routes. But the dynamic route seems to supersede the redirects, making them useless.

Here's an experimental redirect. foo.md doesn't exist, and bar.md does exist:

redirects: {
    '/foo': '/bar',
  },

The redirect is ignored, and navigating to /foo results in an error in rendering the dynamic route: Cannot read properties of undefined (reading 'render').

I'm not sure whether or not this issue is based on a misreading of this line in the docs:

Redirects also follow the same rules, but are prioritized last; if there is a file-based route and a redirect with the same route rule, the file-based route is chosen.

[Edit: now updated to say "same route priority level"]

The way I interpret this line is that, since Static routes without path parameters will take precedence over all other routes, if the redirect is a static route, it should be a higher priority than a file-based route with a path or rest parameter. I can't see it making sense any other way.

What's the expected result?

The redirect's destination should be the updated parameter for the dynamic route.

From the example, navigating to /foo should redirect to /bar, thus [...slug] should receive bar as its parameter.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-z4igp1-ulwjc3?file=astro.config.mjs

Participation

  • I am willing to submit a pull request for this issue.
@ematipico
Copy link
Member

ematipico commented Jul 7, 2023

@paulrudy Here's how the proposal tackles the dynamic routes: https://github.com/withastro/roadmap/blob/redirects/proposals/redirects.md#dynamic-routes

I believe your use case is not handled per design.

EDIT: your example doesn't use getStaticPaths, do you mind fixing the repro?

@ematipico ematipico added the needs response Issue needs response from OP label Jul 7, 2023
@paulrudy
Copy link
Contributor Author

paulrudy commented Jul 7, 2023

@paulrudy Here's how the proposal tackles the dynamic routes: https://github.com/withastro/roadmap/blob/redirects/proposals/redirects.md#dynamic-routes

I believe your use case is not handled per design.

@ematipico The example in the proposal seems to be the opposite of my case:

For example, you could have a file system route /src/pages/blog/contributing.astro and a redirect route /blog/[...slug]. If these were both filesystem routes you would expect contributing.astro to be prioritized over the spread route. This is the case with redirects a well.

Unless I'm misunderstanding something, in the example, the named route takes precedence over the spread route. That matches the route priority order. Im my case, the spread filesystem route seems to be taking precedence over the named route in the redirect! That doesn't match the route priority order.

EDIT: your example doesn't use getStaticPaths, do you mind fixing the repro?

I didn't see anywhere that getStaticPathsis a requirement for redirects. Could you explain?

@matthewp matthewp self-assigned this Jul 12, 2023
matthewp added a commit that referenced this issue Jul 13, 2023
matthewp added a commit that referenced this issue Jul 13, 2023
* Update redirects static generation based on recs

Got some great recommendations on how to handle our HTML written
redirect code based on SEO best practices.

See withastro/roadmap#466 (comment)

This implements them all.

* Fix for using the root path / as a redirect

Fixes #7478

* Fix static redirects prefer over dynamic page

Fixes #7581
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Issue needs response from OP
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants