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

Wrong params and route when index is server side rendered #9757

Closed
1 task
zoxon opened this issue Jan 22, 2024 · 15 comments
Closed
1 task

Wrong params and route when index is server side rendered #9757

zoxon opened this issue Jan 22, 2024 · 15 comments
Labels
needs response Issue needs response from OP needs triage Issue needs to be triaged

Comments

@zoxon
Copy link

zoxon commented Jan 22, 2024

Astro Info

Astro                    v4.2.1
Node                     v18.18.0
System                   Linux (x64)
Package Manager          unknown
Output                   hybrid
Adapter                  @astrojs/node
Integrations             none

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

Chrome, Firefox, Safari

Describe the Bug

Open page /en/about

In case when page [...lang]/index is SSR rendered page other pages in this folder works wrong
For example page [...lang]/about is renders [...lang]/index with lang Astro.params.lang = en/about

Change export const prerender = false to true will fix this problem

In astro version 3 this works as expected

What's the expected result?

Render about [...lang]/about with Astro.params.lang = en

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-zvp3wm

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 Jan 22, 2024
@michaellocher
Copy link

I've identified the same problem with v4.2.1. There must me an issue with the latest version here.

@ematipico
Copy link
Member

I've identified the same problem with v4.2.1. There must me an issue with the latest version here.

Do you mean that in v4.2.0 this worked as expected?

@michaellocher
Copy link

yes, in my project i've used the pinned v4.2.0 and it behaves as before the update…

@bis0072
Copy link

bis0072 commented Jan 22, 2024

same problem

@Fryuni
Copy link
Member

Fryuni commented Jan 22, 2024

Hey everyone, sorry for the inconvenience this has caused.

This was working previously only due to an inconsistency to how some routes were prioritized, being considered higher priority for some things and lower priority for other things. Specially when rest parameters were involved.

Defining how routes are ordered without relying on file-system details revealed those inconsistencies that Astro and users were relying on without noticing. For example, an /blog/index.astro sometimes behaved as /blog and sometimes as /blog/index.

You can see the opposite problem on #9726 that was caused by 4.2.0, the fix for that on 4.2.1 cause this problem here.

We are discussing whether the index page should have a special behavior that handles this case.

For now, the easiest workaround is to use /[...lang].astro instead of /[...lang]/index.astro as we reverted the change making them behave the same. See an example here.

@mb21
Copy link

mb21 commented Jan 22, 2024

@Fryuni are you sure this is not a bug? I have a similar case: minimal Stackblitz repro.

But basically pages/[...slug]/index.astro takes precedence over pages/health.astro, which seems wrong?

image

Unfortunately, the docs don't really spell out what happens if some of the rules in the precedence order are in conflict...

@Fryuni
Copy link
Member

Fryuni commented Jan 22, 2024

But basically pages/[...slug]/index.astro takes precedence over pages/health.astro, which seems wrong?

The first rule in your link already explains why the index wins in your scenario; it has more segments. Those rules are applied in order. The same solution of using pages/[...slug].astro also applies in your scenario; with the same number of segments, the static route will be used.

Using rest parameters anywhere but the last part of the path will lead to confusion because it creates a more specific route (which is a higher priority) that matches a less specific, shorter path.

As I mentioned, this problem already existed, and some people just happened not to hit it because they relied on an inconsistency. For example, consider these files:

  • pages/[...lang]/about.astro
  • pages/[category].astro

What should the /about page render? Is it an about category or an "about" page with no language?
The pages/[...lang]/about.astro page with lang === undefined will win. And your intention could be either option, especially on the conflict with pages/[category].astro. Astro itself can't know what your intention was with those.

This problem was already present in previous versions of Astro. The difference is that it happened due to an implementation detail and not because of an established rule.

There are possible solutions for the index page, but for the example above there isn't a solution that is always right.

@michaellocher
Copy link

Hey @Fryuni i did not use the ... operator in my routes. i've just the following tree:

pages/
├─ [id]/
│  ├─ [title]/
│  │  ├─ index.astro
├─ admin/
│  ├─ index.astro
│  ├─ login.astro
│  ├─ logout.astro
index.astro

If i call the /admin/login route then the one with the [id]/[title] will be called. but i think, that admin/login is way more specific and should be used?

@Fryuni
Copy link
Member

Fryuni commented Jan 22, 2024

That is still 3 segments vs 2 segments. But thank you, I'm taking notice of these use cases to propose a fix that makes the intuitive sense for most cases even though it won't be the most intuitive for all cases.

Your case replacing [id]/[title]/index.astro with [id]/[title].astro should also serve as a workaround for now :)

@michaellocher
Copy link

Yes, this workaround will do the trick for now.

@zoxon
Copy link
Author

zoxon commented Jan 23, 2024

I think it would be better if developers had the option to create and extend routes manually in complex cases
I can't use [lang].astro because lang is optional in my project and I need routes without it (/about, /blog etc)

@Fryuni
Copy link
Member

Fryuni commented Jan 24, 2024

Seems I forgot to link this issue. It should be fixed by #9786 as well

@florian-lefebvre
Copy link
Member

Upgrading reproduction's astro version to 4.2.4 seems to fix the issue (https://stackblitz.com/edit/github-zvp3wm-3rzwgs?file=package.json). Does that work for you @zoxon?

@florian-lefebvre florian-lefebvre added the needs response Issue needs response from OP label Jan 24, 2024
@zoxon
Copy link
Author

zoxon commented Jan 26, 2024

It works for me, thank you. But there is another problem with @astrojs/node in middleware mode. Looks like v4 is too buggy to update it now.

@zoxon zoxon closed this as completed Jan 26, 2024
@florian-lefebvre
Copy link
Member

Sorry about that, we're really making our best! Feel free to open another issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Issue needs response from OP needs triage Issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

7 participants