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

trailingSlash and ERR_TOO_MANY_REDIRECTS on Vercel #10011

Closed
1 task
florian-lefebvre opened this issue Feb 7, 2024 · 8 comments · Fixed by #10082
Closed
1 task

trailingSlash and ERR_TOO_MANY_REDIRECTS on Vercel #10011

florian-lefebvre opened this issue Feb 7, 2024 · 8 comments · Fixed by #10082
Labels
- P2: has workaround Bug, but has workaround (priority) pkg: vercel Related to Vercel adapter (scope)

Comments

@florian-lefebvre
Copy link
Member

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.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Feb 7, 2024
@ematipico ematipico added needs response Issue needs response from OP and removed needs triage Issue needs to be triaged labels Feb 7, 2024
@ematipico
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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

@ematipico
Copy link
Member

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

@ematipico ematipico added needs discussion Issue needs to be discussed and removed needs response Issue needs response from OP labels Feb 8, 2024
@florian-lefebvre
Copy link
Member Author

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
Copy link
Member

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.

@ematipico ematipico added - P2: has workaround Bug, but has workaround (priority) pkg: vercel Related to Vercel adapter (scope) and removed needs discussion Issue needs to be discussed labels Feb 8, 2024
@vntw
Copy link

vntw commented Jul 31, 2024

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:

  1. 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
  2. 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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: has workaround Bug, but has workaround (priority) pkg: vercel Related to Vercel adapter (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants