trailingSlash and ERR_TOO_MANY_REDIRECTS on Vercel

florian-lefebvre commented 9 months ago

Astro Info

Astro                    v4.3.3
Node                     v18.19.0
System                   Windows (x64)
Package Manager          pnpm
Output                   hybrid
Adapter                  @astrojs/vercel/serverless
Integrations             @astrojs/mdx
                         @astrojs/sitemap

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

No response

Describe the Bug

Using the vercel adapter, setting trailingShlash: "always" (astro config) + "trailingSlash": true (vercel config) can make some routes to throw a ERR_TOO_MANY_REDIRECTS. For this repro, it happens on the /blog/ page but on another private project I'm working on, it does it for the favicon and sitemap generated urls.

https://astro-too-many-redirects-repro.vercel.app/blog/

What's the expected result?

I expect such configuration to not throw this error.

Link to Minimal Reproducible Example

https://github.com/florian-lefebvre/astro-too-many-redirects-repro

Participation

[ ] I am willing to submit a pull request for this issue.

ematipico commented 9 months ago

I expect such configuration to not throw this error.

That's a bit generic as an expectation. Since both configurations go in conflict (astro and vercel), which one should win?

florian-lefebvre commented 9 months ago

Ah I see what you mean. I guess I'd expect vercel to redirect from /blog to /blog/ and Astro to return a 404 if /blog is ever reached?

florian-lefebvre commented 9 months ago

Or should I remove the trailingSlash config in Astro to let Vercel handle it?

ematipico commented 9 months ago

IMHO, a configuration like this should error and never build, because Astro and Vercel adapter don't know what the user wants, due to conflicting options.

Although, I'm not sure if the adapter ever reads vercel.json. If it does, it should do this check

florian-lefebvre commented 9 months ago

Okay so after thinking about it, I ended up doing this:

const DEV = process.env.NODE_ENV === "development"

export default defineConfig({
    trailingSlash: DEV ? 'always' : 'ignore'
})

This way I get a consistent behavior between dev and prod. I think it could be great if the vercel integration looked for a vercel.json in the root and based on trailingSlash, overwrites it using updateConfig. Tell me if that sounds good and I could submit a PR

ematipico commented 9 months ago

Okay so after thinking about it, I ended up doing this:
const DEV = process.env.NODE_ENV === "development"

export default defineConfig({
  trailingSlash: DEV ? 'always' : 'ignore'
})
This way I get a consistent behavior between dev and prod. I think it could be great if the vercel integration looked for a vercel.json in the root and based on trailingSlash, overwrites it using updateConfig. Tell me if that sounds good and I could submit a PR

That sounds like a reasonable approach to me! I would also add a warning while doing so, because we still don't know which trailing slash is the correct one and users should know that the adapter will use the one option defined in the vercel.json.

vntw commented 3 months ago

Sorry for digging up an old issue, but I'm confused about some details, maybe someone could help me clear them up? I wanted to set this up exactly like OP mentioned:

Get a 404 with the dev server if a trailing slash is missing. According to the docs for the trailingSlash option "Set the route matching behavior of the dev server" – I didn't expect this to have any effect on anything but the dev server
Have vercel redirect from /blog to /blog/ in case it's missing somewhere and for consistent links, also in regards to SEO indexing and duplicates

So I configured Astro to use trailingSlash: "always" and vercel trailingSlash: true. How would this conflict? Don't these configs try to achieve the same goal, just in different envs?

Does the Astro config affect more than the dev server, contrary to the docs? I could understand it if the settings were sth. like Astro trailingSlash: "always" and vercel trailingSlash: false which sounds like an actual conflict to me.

I must be missing sth., I'd be grateful for any input here 😅

florian-lefebvre commented 3 months ago

Indeed both conflict! In the linked PR, if you are in such situation (in build only I think but not sure), the adapter will warn you and update the astro config to trailingSlash: 'ignore'

https://github.com/withastro/astro/pull/10082/files#diff-071ccfbdfd7b9bc803219179f88ae0bdb227a399bc4a481b7831c82b801d7a17

withastro / astro