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

Configured Redirects with more than two params don't expand properly #8914

Closed
1 task
FredericLatour opened this issue Oct 24, 2023 · 12 comments · Fixed by #9089
Closed
1 task

Configured Redirects with more than two params don't expand properly #8914

FredericLatour opened this issue Oct 24, 2023 · 12 comments · Fixed by #9089
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: redirects Related to the redirects feature (scope)

Comments

@FredericLatour
Copy link

FredericLatour commented Oct 24, 2023

Astro Info

Astro                    v3.3.2
Node                     v18.17.1
System                   Linux (x64)
Package Manager          pnpm
Output                   server
Adapter                  @astrojs/node
Integrations             @astrojs/tailwind
                         @astrojs/mdx
                         @astrojs/react

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Context

Redirects configuration in astro.config.mjs

Tested usint the following configuration (however certainly not limited to this configuration):

  • SSR mode
  • Node adapter

Bug

Redirects entries with more than one param (ie: '/ov/[page]/[...user]': '/tests/ov/[page]/[...user]') don't work.
The expansion does not take place.

Example:

Given the following configuration:

export default defineConfig({
  redirects: {
    '/ov/[page]/[...user]': '/tests/ov/[page]/[...user]', // does not work
    '/ot/[...user]': '/tests/ot/[...user]',               // works
  },
  integrations: [tailwind(), mdx(), react()],
  output: "server",
  adapter: node({
    mode: "standalone",
  }),
})

While

What's the expected result?

Given the configuration above,

Link to Minimal Reproducible Example

https://github.com/lilnasy/redirect-issue

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Oct 24, 2023
@lilnasy lilnasy added - P3: minor bug An edge case that only affects very specific usage (priority) feat: redirects Related to the redirects feature (scope) and removed needs triage Issue needs to be triaged labels Oct 24, 2023
@FredericLatour
Copy link
Author

While trying to find a workaround (basically, using a single rest route), I realized that the problem was somewhat more complex than the issue would make it think.

For instance, even when a single param 'route'/'redirect config' (a rest one though) is concerned, the redirection may not work.

For instance with the following redirects configuraiton

'/ot/old-page/[...pageuser]': '/play/ot/new-page/[...pageuser]',

Not sure how to complete, modify the issue to somewhat extend the scope of the problem.

@rishi-raj-jain
Copy link
Contributor

@lilnasy

http://localhost:4321/play/ot/new-page/[...pageuser]
Is this a known issue with redirection? From the description it seems like there are two issues:

  • redirects do not take place with both single level dynamic and nested routes
  • even when redirects do take place, they fail to pass on the dynamic slug to the redirected path

@lilnasy
Copy link
Contributor

lilnasy commented Nov 10, 2023

Yeah, seems like it. Although the two issues may have the same cause: "..." being included in some match and replace maybe.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 10, 2023

While trying to find a workaround (basically, using a single rest route), I realized that the problem was somewhat more complex than the issue would make it think.

I don't think that is the case.

I think what you may be missing is the fact that the target route needs to exist for the expansion to work.

For example, for this redirect rule to expand properly...

'/ot/old-page/[...pageuser]': '/play/ot/new-page/[...pageuser]',

...the target route named src/pages/play/ot/new-page/[...pageuser].astro or .ts needs to exist.

@nastiazhyrnova
Copy link

nastiazhyrnova commented Nov 13, 2023

I think I may have similar issue (using storyblok CMS + i18n)

'/de-de/services': '/de/services' doesn't work, as well as '/de-de/[...slug]': '/de/[...slug]' , which is what I actually need
but
'/de-de': '/de' works
'/': '/de' works

PS both '/de-de' and '/de' paths are generated in getStaticPaths() in [...slug].astro

@lilnasy
Copy link
Contributor

lilnasy commented Nov 13, 2023

@nastiazhyrnova Can you share what adapter you are using, and what your src/pages folder looks like? I'm specifically interested in whether a file called src/pages/de/[...slug].astro exists.

@nastiazhyrnova
Copy link

@lilnasy adapter is

adapter: env.SSR === 'true' ? node({ mode: 'standalone', }) : undefined,

env.SSR I tried both false and true, it doesn't change anything in this case...

Regarding the folder structure, it is actually src/pages/[...slug].astro, it works for both english and german versions.
There is src/pages/en/[...id].astro and src/pages/de/[...id].astro but it is a different component for another functionality

@lilnasy
Copy link
Contributor

lilnasy commented Nov 13, 2023

Thanks, this really helps.
I think the workaround would be to create empty files whose name matches the redirect targets. src/pages/de/[...slug].astro in your case.

@nastiazhyrnova
Copy link

Hi @lilnasy I've tried it, but unfortunately it doesn't work :( Moreover, if I also remove [...slug].astro from src/pages, nothing works, which probably means that another [...slug].astro files are not being considered :(

@rishi-raj-jain
Copy link
Contributor

@lilnasy

How about we:

  • let the server handle the redirect instead of looking for files redirect to
  • if available, wherever redirect resolves to, send the file else a 404

@nastiazhyrnova
Copy link

@rishi-raj-jain so far I see it as an only solution, in my case it is some middleware in Vercel, have to have a look at it

@lilnasy
Copy link
Contributor

lilnasy commented Nov 14, 2023

That is the plan. The logic would go here: #9089 (comment).
To clarify, I only made the PR to help discuss the best way to refactor. Feel free to open a new one, and maybe copy the tests.

The information we have is data = { slug: "x/y/z" }, and route = "/de/[...slug]". The result we need is generate(route, data) === "/de/x/y/z".

Normally, this generate function is already there but it comes from the target route. In this case, the target route is named differently so it is not available.

Ideally, we would refactor routeData.generate so that this function is not duplicated, but if there's no easy way, a simple RegExp replace in the link would get the job done too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: redirects Related to the redirects feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants