Redirects RFC #587

matthewp · 2023-05-22T14:09:08Z

Summary

Provide a redirects config option for specifying redirected pages. Provide good integration with adapters.

Links

proposals/redirects.md

- Show object notation syntax. - Explain dynamic routes. - Specify the other status code.

proposals/redirects.md

tony-sull · 2023-05-23T18:33:57Z

Notes from May 23 community call:

what takes priority when src/pages, injectRoute, and redirects collide?
- currently redirects takes the highest priority
- should routing priority follow the same rules with regards to rest routes? i.e. will src/pages/blog/contributing.astro be overwritten by a redirect rule for blog/[...slug]?

delucis

Excited for this API — thanks for putting in the work to flesh it out @matthewp!

proposals/redirects.md

delucis · 2023-05-25T13:43:22Z

proposals/redirects.md

+
+```js
+interface RedirectConfig = string | {
+  status: 300 | 301 | 302 | 303 | 304 | 307 | 308;


I know some hosts also support a 200 redirect. For example from the Netlify redirect docs:

200: OK code. Redirects with this status code will change the server response without changing the URL in the browser address bar. This is used for rewrites and proxying.

Is there a reason for us preventing that here?

Is this a Netlify specific thing? I can't find any info on the web in general about 200 being used for Redirects.

I guess we could open it up to number entirely and leave it on users to provide a valid value.

Looks like Cloudflare does this too: allows 200 for “proxying” and 404 for “rewrites”. Pretty similar to Netlify (I forgot they support 404 too).

On Vercel, redirects and rewrites are handled separately and looks like Netlify/Cloudflare’s 200 redirect is what Vercel calls a “rewrite”: https://vercel.com/docs/concepts/edge-network/rewrites

Not really familiar with Deno Deploy and don’t see anything obvious in their docs. I guess there it’s closer to our Node adapter? So up to us how exactly we handle stuff.

Looks like the static site platforms use a “200 redirect” to mean a server-side rewrite where the destination content gets served at the requested URL instead of notifying about a redirect. Probably useful for SPAs in particular.

delucis · 2023-05-25T13:45:22Z

proposals/redirects.md

+```html
+`<!doctype html>
+<title>OLD_LOCATION</title>
+<meta http-equiv="refresh" content="0;url=LOCATION" />


We can’t set status codes with the <meta> tag approach, right? Would we document that configured status codes only apply to sites hosted on platforms that support that?

Correct, we can't apply status codes here. So when deploying to a host without configuration you're always going to get a 200 response from these pages. So no effect on SEO, unfortunately. Yeah we can and should document that.

delucis · 2023-05-25T13:46:00Z

proposals/redirects.md

+
+export default defineConfig({
+  redirects: {
+    '/blog/[...slug]': '/team/articles/[...slug]'


Oh this is smart!

proposals/redirects.md

delucis · 2023-05-25T14:02:07Z

proposals/redirects.md

Agree that we should figure out priority. It’s not clear to me what’s best. I’d probably land on user route > injectRoute > redirect. That means you can’t redirect away from an existing page — you must remove the page in order to use a redirect — but that mirrors how platforms often work: Netlify for example ignores redirect config for pages that exist in the build output.

In fact, we should double check static output doesn’t interfere with redirects configured in the Netlify adapter. We might need to turn off the static meta tag output there. We have this issue in docs where because we have routes with meta redirects, we lose Netlify redirects, e.g. / route — view-source:https://docs.astro.build/ — despite having them configured.

I just updated the text to explain priority. I was able to make it so that it has the same priority as FS routes.

Got it. Couple points there then:

In the case that a redirect and a file-system route of the same priority type exists (both spread or both not spread routes), which wins is not specified. I’d suggest the redirect loses as per platform behaviour mentioned above.

We likely still need to have a way to avoid building the static <meta> routes if the adapter supports redirect config: Netlify will definitely ignore a config redirect in favour of a matching HTML file, and from reading Vercel’s docs, I suspect they do too: “precedence is given to the filesystem prior to rewrites being applied”

I can do some more research on that if you need.

Let's take the point about <meta> to another comment, here: #587 (comment)

matthewp · 2023-05-25T14:04:40Z

Note that this was just updated to explain routing priority. The intent is for it to have the same priority as filesystem routes. If there is an exact match then the redirect would win. But in the case of, for example, siblings with the redirect being a spread, the filesystem non-spread is prioritized, just as it would be if they were both FS based.

Co-authored-by: Chris Swithinbank <[email protected]>

matthewp · 2023-05-25T19:28:20Z

@delucis says:

We likely still need to have a way to avoid building the static routes if the adapter supports redirect config: Netlify will definitely ignore a config redirect in favour of a matching HTML file, and from reading Vercel’s docs, I suspect they do too: “precedence is given to the filesystem prior to rewrites being applied”

I've thought about this and this is a bit problematic. We don't currently have a mechanism for an adapter to prevent a route from being built. A few possible solutions:

Have some sort of special-case option for redirects ala writeRedirects: false. Don't love it.
Provide a way to filter out files before they are written. This is more generic but feels complicated if there's not more use-cases.
The integrations could just delete the files themselves.

I'm leaning towards (3) but if this is something all adapters need to do then that doesn't feel great either.

matthewp · 2023-05-25T19:50:41Z

config.build.redirects = false wouldn't be terrible either, I guess.

delucis · 2023-05-26T06:26:15Z

Yeah a config flag adapters can toggle off doesn't seem too bad. A fourth option would be to just do this implicitly when an adapter is set on the basis that if there is an adapter, redirects are their responsibility. But I think a flag might be safest for the case where an adapter doesn't handle this.

matthewp · 2023-05-30T12:46:40Z

There are likely adapters that do not have config based redirects, so I wouldn't want to blanket disable the feature on the presence of an adapter. I think I'll go with the config approach. Will update the RFC and implementation.

matthewp · 2023-05-30T18:14:56Z

@delucis Updated to include the config: dfd349e

delucis · 2023-05-30T19:48:18Z

Thanks! LGTM 👍

I think the outstanding points from above are:

Do we widen valid values for status? (e.g. to include 200 or 404 or just any number and it’s a user’s job to pick a good one 😅)
If a redirect and a file-system route of the same priority type exists (i.e. both spread or both not-spread routes), which wins? I suggested the redirect loses.

matthewp · 2023-05-30T21:57:43Z

I don't think we should widen the value of status. The 200 example seems to be a special case for Netlify and not a widely supported thing. In that case it belongs in the project's _redirects file (which gets merged with redirects config). It wouldn't make sense to put this sort of thing into the config because it's not portable; the point of this feature is having a portable place to put redirects.
Redirects losing sounds good to me, if this is what other systems do. I'll update the RFC to reflect that.

matthewp · 2023-05-30T22:16:58Z

Updated to specify that FS routes win out when there are exact matches.

delucis · 2023-05-30T22:53:57Z

I don't think we should widen the value of status. The 200 example seems to be a special case for Netlify

I’m fine with this. I agree sticking to the common features to maintain cross-platform compatibility is a good idea. For the record, something like this is also supported by Cloudflare and Vercel (see #587 (comment)).

No more comments from me. Thanks for putting up with all my feedback! 😅

matthewp · 2023-05-31T00:12:39Z

I'd be happy to open this up if we get feedback that people expect this. Will do some more research to see what the support matrix of these codes look like.

jlarmstrongiv · 2023-06-08T05:07:57Z

We’re having trouble with the new experimental redirects in withastro/astro#7322:

{
  experimental: {
    redirects: true
  },
  redirects: {
    '/a': '/b'
  }
}

I think this syntax matches the RFC proposal

matthewp · 2023-07-13T14:37:42Z

I'm calling for consensus on this RFC. This will now enter the final comment period. If there are any objections to this feature being unflagged as stable please state so now. After 3 days if there are no objections this RFC will be merged.

matthewp · 2023-07-17T14:39:28Z

Consensus passes, this RFC is accepted.

Redirects RFC

34f91fc

matthewp mentioned this pull request May 22, 2023

Redirects withastro/docs#3311

Closed

bluwy reviewed May 22, 2023

View reviewed changes

proposals/redirects.md Outdated Show resolved Hide resolved

proposals/redirects.md Show resolved Hide resolved

ematipico reviewed May 23, 2023

View reviewed changes

proposals/redirects.md Outdated Show resolved Hide resolved

matthewp added 2 commits May 23, 2023 08:53

Updates:

3aa1322

- Show object notation syntax. - Explain dynamic routes. - Specify the other status code.

Specify 308 for non-GET

c043f24

matthewp mentioned this pull request May 23, 2023

Redirects withastro/astro#7067

Merged

natemoo-re reviewed May 23, 2023

View reviewed changes

proposals/redirects.md Outdated Show resolved Hide resolved

Update proposals/redirects.md

c16aa45

matthewp mentioned this pull request May 23, 2023

Use status 308 for non-GET redirects withastro/astro#7186

Merged

delucis reviewed May 25, 2023

View reviewed changes

Specify routing priority

9e2d0eb

Update proposals/redirects.md

fb0a86d

Co-authored-by: Chris Swithinbank <[email protected]>

Update text to reflect disabling of the HTML generation

dfd349e

Specify that FS routes override redirects

fff55c3

astrobot-houston mentioned this pull request Jun 5, 2023

[ci] release withastro/astro#7295

Merged

matthewp merged commit 109f356 into main Jul 17, 2023

matthewp deleted the redirects branch July 17, 2023 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirects RFC #587

Redirects RFC #587

matthewp commented May 22, 2023 •

edited

Loading

tony-sull commented May 23, 2023

delucis left a comment

delucis May 25, 2023

matthewp May 25, 2023

delucis May 25, 2023

delucis May 25, 2023 •

edited

Loading

delucis May 25, 2023

delucis May 25, 2023 •

edited

Loading

matthewp May 25, 2023

delucis May 25, 2023

delucis May 25, 2023

matthewp May 25, 2023

delucis May 25, 2023

matthewp May 25, 2023

matthewp commented May 25, 2023

matthewp commented May 25, 2023

matthewp commented May 25, 2023

delucis commented May 26, 2023

matthewp commented May 30, 2023

matthewp commented May 30, 2023

delucis commented May 30, 2023 •

edited

Loading

matthewp commented May 30, 2023

matthewp commented May 30, 2023

delucis commented May 30, 2023

matthewp commented May 31, 2023

jlarmstrongiv commented Jun 8, 2023

matthewp commented Jul 13, 2023

matthewp commented Jul 17, 2023

Redirects RFC #587

Redirects RFC #587

Conversation

matthewp commented May 22, 2023 • edited Loading

Summary

Links

tony-sull commented May 23, 2023

delucis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delucis May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delucis May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewp commented May 25, 2023

matthewp commented May 25, 2023

matthewp commented May 25, 2023

delucis commented May 26, 2023

matthewp commented May 30, 2023

matthewp commented May 30, 2023

delucis commented May 30, 2023 • edited Loading

matthewp commented May 30, 2023

matthewp commented May 30, 2023

delucis commented May 30, 2023

matthewp commented May 31, 2023

jlarmstrongiv commented Jun 8, 2023

matthewp commented Jul 13, 2023

matthewp commented Jul 17, 2023

matthewp commented May 22, 2023 •

edited

Loading

delucis May 25, 2023 •

edited

Loading

delucis May 25, 2023 •

edited

Loading

delucis commented May 30, 2023 •

edited

Loading