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

config.redirects does not handle trailing slashes #12532

Open
1 task
tordans opened this issue Nov 26, 2024 · 10 comments
Open
1 task

config.redirects does not handle trailing slashes #12532

tordans opened this issue Nov 26, 2024 · 10 comments
Labels
needs discussion Issue needs to be discussed

Comments

@tordans
Copy link
Contributor

tordans commented Nov 26, 2024

Astro Info

% npx astro info
Astro                    v4.16.14
Node                     v22.9.0
System                   macOS (arm64)
Package Manager          npm
Output                   ~hybrid~ // this was tested using the build version which uses `static`
Adapter                  none
Integrations             keystatic
                         @astrojs/tailwind
                         @astrojs/react
                         @astrojs/mdx
                         @astrojs/sitemap
                         astro-matomo

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

Chrome

Describe the Bug

Context

I have an existing site that used to link to /route and /route/ which both rendered the same page.
I am now migrating this site to Astro and want a clean URL structure for both SEO but also to do internal "current page" checks (without looking at both variations).

Goal

Use the redirects config to redirect the trailing slash URLs to the non trailing slash URL.

Test 1

  output: 'static',
  redirects: {
    '/route/': '/route',
  },
  // trailingSlash: 'never',
  // build: { format: 'file' },
  • Redirect of /route/ works ✅ :
Details
% http http://localhost:4321/route/
HTTP/1.1 200 OK

<!doctype html><title>Redirecting to: https://radschnellweg-frm7.de/route</title><meta http-equiv="refresh" content="0;url=https://radschnellweg-frm7.de/route"><meta name="robots" content="noindex"><link rel="canonical" href="https://radschnellweg-frm7.de/route"><body>	<a href="https://radschnellweg-frm7.de/route">Redirecting from <code>/route/</code> to <code>https://radschnellweg-frm7.de/route</code></a></body>
  • BUT /route also gets redirected to /route 🔴 :
Details
% http http://localhost:4321/route
HTTP/1.1 200 OK

<!doctype html><title>Redirecting to: https://radschnellweg-frm7.de/route</title><meta http-equiv="refresh" content="0;url=https://radschnellweg-frm7.de/route"><meta name="robots" content="noindex"><link rel="canonical" href="https://radschnellweg-frm7.de/route"><body>	<a href="https://radschnellweg-frm7.de/route">Redirecting from <code>/route/</code> to <code>https://radschnellweg-frm7.de/route</code></a></body>

Test 2

Following suggestions from #7808 I tried other config options…

  output: 'static',
  redirects: {
    '/route/': '/route',
  },
  trailingSlash: 'never',
  build: { format: 'file' }, // adding or not adding this does not change the outcome
  • Redirect of /route/ fails with trailingSlash is set to "never" 🔴 :
Details
% http http://localhost:4321/route/
HTTP/1.1 404 Not Found

<!doctype html>
<html lang="en">
	<head>
		<meta charset="UTF-8">
		<title>404: Not Found (trailingSlash is set to "never")</title>
…
  • And /route still fails by redirecting to /route 🔴 :
Details
% http http://localhost:4321/route
HTTP/1.1 200 OK

<!doctype html><title>Redirecting to: https://radschnellweg-frm7.de/route</title><meta http-equiv="refresh" content="0;url=https://radschnellweg-frm7.de/route"><meta name="robots" content="noindex"><link rel="canonical" href="https://radschnellweg-frm7.de/route"><body>	<a href="https://radschnellweg-frm7.de/route">Redirecting from <code>/route/</code> to <code>https://radschnellweg-frm7.de/route</code></a></body>

What's the expected result?

  1. I expected redirects: { '/route/': '/route', }, to include the trailing slash in the redirect lookup and only redirect if it is an exact match. I think this should always happen, regardless of the trailingSlash setting.
  2. I expected redirects: { '/route/': '/route', }, trailingSlash: 'never', to redirect the /route/ and not show a 404.
  3. I think the redirect docs should clarify that redirecting directory to files are not supported, yet

Link to Minimal Reproducible Example

#12532 (comment)

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 Nov 26, 2024
@tordans
Copy link
Contributor Author

tordans commented Nov 26, 2024

@gvkhna given your comment at #7808 (comment) you might have insights on this and maybe found a way to work around this, already?

@ematipico
Copy link
Member

ematipico commented Nov 26, 2024

BUT /route also gets redirected to /route

I am not sure I understand why this case should be considered, it adds noise in the triaging process. I expect this to not happen, otherwise you should be stuck into a infinite redirect loop.


redirects are meant to redirect existing old routes to existing new routes. When you enable trailingSlash: 'never', ALL routes in your site must have a slash at the end. Routes that don't have a trailing slash are considered non-existent.

Since /route/ doesn't exist, Astro can't perform a redirect.

Considering your niche case, I think you should use a middleware to make the redirect yourself. Does it make sense?

@ematipico ematipico added the needs response Issue needs response from OP label Nov 26, 2024
@tordans
Copy link
Contributor Author

tordans commented Nov 26, 2024

@ematipico thanks for your quick reply 🙏

Considering your niche case

Let's maybe start here: Is it really a niche case?

  • Are we in agreement that having both URIs (/route/, /route) is bad for SEO and DX (like template code that checks the current URL Astro.url.pathname === '/route')?
  • Are we in agreement that many sites are migrated to Astro that did not handle those cases well before, so now the new site (Astro) has to clean this up while still supporting both URIs (by guiding one to the other…)
  • Are we in agreement that even in new Astro site, users might still add a trailing slash to a URL for example in a Markdown, so the site has to handle those cases…

Given this, I think there should be a recommended/documented way to set this up properly.

I think you should use a middleware to make the redirect yourself.

I am trying hard to stay in static land here, so this would not be an option. I can work around this with just JS and canonical tags.

BUT /route also gets redirected to /route
(…) I expect this to not happen, otherwise you should be stuck into a infinite redirect loop.

Agreed, I also expected this not to happen, but it does.

Which is why I was confused to find that redirects: { '/route/': '/route', }, does trigger for /route.

redirects are meant to redirect existing old routes to existing new routes.

The thing is, that /route/index.html and /route.html are different pages. I understand the redirect config as…

  • old route: /route/ (/route/index.html)
  • existing new route: /route (/route.html)

(I don't understand what you mean by "existing old routes" in my mind every old route is non-existing.)

When you enable trailingSlash: 'never', ALL routes in your site must have a slash at the end. Routes that don't have a trailing slash are considered non-existent.
Since /route/ doesn't exist, Astro can't perform a redirect.

How can any old route be "existing" in Astros mind?
In my mind, the redirects "old" entry should always create a redirect entry (be it a static file or some other form of redirect). My mental model of the redirects config is, that it is the same as a .htaccess or Netlify Redirect config. Is that wrong?

@ematipico
Copy link
Member

ematipico commented Nov 26, 2024

I am trying hard to stay in static land here

Middleware can also be used for static sites, too. It's documented, which is why I suggested it. It runs during the build and it generates static HTML file based on what you return.

I don't understand what you mean by "existing old routes" in my mind every old route is non-existing.

Astro redirects need a file system route (a file under src/pages/*) to match the keys of the redirects. That's incorrect, sorry. That's on me, I remembered things incorrectly.

The thing is, that /route/index.html and /route.html are different pages.

It really depends on how you want to serve your files and your host provider. With ngnix, for example, both files can be served when navigating the URL path /route

How can any old route be "existing" in Astros mind?
In my mind, the redirects "old" entry should always create a redirect entry (be it a static file or some other form of redirect). My mental model of the redirects config is, that it is the same as a .htaccess or Netlify Redirect config. Is that wrong?

I'll need to investigate that. In the meantime, please provide a minimal reproduction. Without that, I'll have to close the issue eventually.

@ematipico ematipico added needs discussion Issue needs to be discussed needs repro Issue needs a reproduction and removed needs response Issue needs response from OP labels Nov 26, 2024
Copy link
Contributor

Hello @tordans. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Nov 26, 2024
@gvkhna
Copy link

gvkhna commented Nov 26, 2024

Thanks for tagging me @tordans. Yes I still maintain as in my comment you tagged me on here: #7808 (comment) would be the correct behavior for this api.

After my input was ignored I considered this api broken and gave up, as in however it works is what we have and I myself am very unclear what it's supposed to do and what it actually does. Terrible api but they don't seem to want to fix it.

Unfortunately i do agree with @ematipico , if you need specific behavior that isn't supported you'll need to customize through middleware or some other means. I don't think they consider this a bug. However it does work is apparently how it's supposed to work.

@tordans
Copy link
Contributor Author

tordans commented Nov 26, 2024

@ematipico thanks for explaining this more…

I'll need to investigate that. In the meantime, please provide a minimal reproduction. Without that, I'll have to close the issue eventually.

Here is my minimal reproduction:

Test 1

  1. https://stackblitz.com/edit/github-wpywd9?file=src%2Fpages%2Findex.astro,src%2Flayouts%2FLayout.astro,src%2Fpages%2Froute.astro,astro.config.mjs&file=astro.config.mjs&on=stackblitz
      // trailingSlash: 'never',
      // build: { format: 'file' },
    
  2. npm run build
  3. npm run preview
  4. Open https://nbwiywwlnqgithub-pusf--4321--88b6dd69.local-credentialless.webcontainer.io/
    • => 🟢 all the redirects work and redirect to /route
    • => 🔴 but /route is an endless redirect loop instead of showing route.astro
      • My expectation is, that the lookup that check is a redirect should trigger looks at the full Astro.url.pathname

Test 2

  1. https://stackblitz.com/edit/github-wpywd9?file=src%2Fpages%2Findex.astro,src%2Flayouts%2FLayout.astro,src%2Fpages%2Froute.astro,astro.config.mjs&file=astro.config.mjs&on=stackblitz and remove the comments from trailingSlash and build
      trailingSlash: 'never',
      build: { format: 'file' },
    
  2. npm run build
  3. npm run preview
  4. Open https://nbwiywwlnqgithub-pusf--4321--88b6dd69.local-credentialless.webcontainer.io/
    • => 🔴 '/redirect3/': '/route/', fails and does not redirect but shows the custom error message custom "404: Not Found (trailingSlash is set to "never")"
      • the URL stays https://nbwiywwlnqgithub-pusf--4321--88b6dd69.local-credentialless.webcontainer.io/redirect3/
      • but it should perform the redirect to /route/
      • My expectation is that the fact that I have trailingSlash: 'never', should not prevent a redirect from happening…
    • => 🔴 '/redirect1': '/route',, '/redirect2': '/route/',, '/route/': '/route', all fail with the same endless redirect loop from Test 1

@ematipico ematipico removed the needs repro Issue needs a reproduction label Nov 26, 2024
@tordans
Copy link
Contributor Author

tordans commented Dec 12, 2024

Here is a good, recent article that talks about this exact issue that I and @gvkhna are trying to solve, a good User- and SEO-Experience with redirects https://bjornlu.com/blog/trailing-slash-for-frameworks

Via https://bsky.app/profile/zachleat.com/post/3lczynjv4cc2s

It does into details to compare how hoster and server treat the issue.
It also recommends to redirect rather than 404 those URLs that are unwanted – which is what this issue is about.

@gvkhna
Copy link

gvkhna commented Dec 24, 2024

To add to this discussion regarding SEO. The issue I face is around the canonical. It's basically broken unless you custom handle it. We discussed this issue over at jonasmerlin/astro-seo#76

The code referenced here

<link rel="canonical" href={Astro.props.canonical || Astro.url.href} />

Is what is desired. But in both trailing slash pages, and non-trailing slash pages, the canonical url is specified as that version. So you end up with duplicate canonical's which is a huge issue for SEO. Ideally you pick one as the canonical and redirect from the other. But as it stands the Astro.props.canonical and the trailingSlash configuration property together are broken and don't allow this behavior.

@tordans My current workaround is to turn all of this off and handle it correctly through simple but tedious code.

This may also be related: jonasmerlin/astro-seo#101

@my-astro
Copy link

my-astro commented Jan 3, 2025

@tordans I want to figure out this aspect too. But it seems that even if it would have worked correctly on the Astro end once we upload to netlify/cloudflare URL normalization kicks in and serves both bare and trailingslash versions simultaneously. I’ve made some testings and normalization is absent in Astro server mode and you can configure everything as you desire.

But server mode brings a whole new level of complexity and problems.

Simpler way for SEO would be to keep those disgusting trailing slashes in place, cause AJAX, Cloudflare, Netlify follow similar standards and redirect them (pretty urls in netlify, normalization in cloudflare). To note though I would still do 301 redirects in cloudflare manually, cause they server 308 code by default which would result in funny things in Google search console

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Issue needs to be discussed
Projects
None yet
Development

No branches or pull requests

4 participants