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

BUG: injectRoute merge routes in production when entrypoint is already used #10622

Closed
1 task done
goulvenclech opened this issue Mar 31, 2024 · 16 comments · Fixed by #10625
Closed
1 task done

BUG: injectRoute merge routes in production when entrypoint is already used #10622

goulvenclech opened this issue Mar 31, 2024 · 16 comments · Fixed by #10625
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: astro Related to the core `astro` package (scope)

Comments

@goulvenclech
Copy link
Contributor

goulvenclech commented Mar 31, 2024

Hi everyone 👋

Duplicate my message from #3602 , with two minimal reproductions.

I'm currently making an Astro integration where my injectRoutes works perfectly in dev mode, but where all my paths are weirdly merged in production, even with different pattern. This is caused by using the same .astro entrypoint for two or more injectRoutes.

Astro Info

Astro                    v4.5.12
Node                     v20.10.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   static
Adapter                  none
Integrations             astropi

Simple example with static path

Link to Minimal Reproducible Example -> https://stackblitz.com/edit/github-l3a17a?file=astro.config.mjs

    injectRoute({
      pattern: `/blog`,
      entrypoint: './src/entrypoint.astro',
      prerender: true,
    });
    injectRoute({
      pattern: `/another-blog`,
      entrypoint: './src/entrypoint.astro',
      prerender: true,
    });

^ This will work in dev. But in prod, only /blog/index.html is generated.

Complete example with static & spread paths

To clarify the bug, I have made another (not so) minimal repro -> https://stackblitz.com/edit/github-l3a17a?file=src%2F%5B...slug%5D.astro

This one have spread routes like :

          injectRoute({
            pattern: `/blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });

In dev, everything work as intended, with 5 pages generated with the correct URLs :

/ <- src/pages/index.astro
/blog <- src/blog.astro
/another-blog <- src/blog.astro
/blog/test <- src/[...slug].astro
/another-blog/another-test  <- src/[...slug].astro

Here how it's merged in build :

generating static routes 
12:06:19 ▶ src/blog.astro
12:06:19   └─ /blog/index.html (+24ms)
12:06:19 ▶ src/[...slug].astro
12:06:19   ├─ /blog/test/index.html (+29ms)
12:06:19   └─ /blog/another-test/index.html (+6ms)
12:06:19 ▶ src/pages/index.astro
12:06:19   └─ /index.html (+1ms)
12:06:19 ✓ Completed in 176ms.

We loose one the blog's index. And one article is merged into the wrong URL.

What's the expected result?

Firstly I don't see why the entrypoints influence the generation of routes, when the pattern and the collections are different. Secondly, because the behaviour differs between development and production, so if this issue is closed "as not planed" we should at least limit the behaviour in development with a warning.

Participation

@github-actions github-actions bot added the needs triage Issue needs to be triaged label Mar 31, 2024
@goulvenclech goulvenclech changed the title BUG: injectRoute merge routes if entrypoint is already used BUG: injectRoute merge routes in production when entrypoint is already used Mar 31, 2024
@Fryuni
Copy link
Member

Fryuni commented Mar 31, 2024

Confirming that this issue happens as described.

This is happening due to the page data collection returning a map from the component (the entrypoint for injected routes) to the route using that component here:

allPages[route.component] = {
component: route.component,
route,
moduleSpecifier: '',
styles: [],
propagatedStyles: new Map(),
propagatedScripts: new Map(),
hoistedScript: undefined,
hasSharedModules: false,
};

To support multiple routes using the same entrypoint, this would need to be an array with all the routes with the same route.component. Changing this will also require changing the internals.routePerComponent to an array and every place that uses it. This affects quite a few places so it is not a small change, but TS should be able to guide you through it.

But before anyone starts implementing it, I think it is worth discussing, as you pointed out, which path should be taken from here:

  • The build should work in this case, so a fix is needed.
  • The build should not work in this case, so the dev server should fail in the same scenario.

@Fryuni Fryuni added - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged labels Mar 31, 2024
@goulvenclech
Copy link
Contributor Author

@Fryuni I think this should be fixed because :

  1. I don't see why the entrypoints should influence the generation of routes, when the patterns are different
  2. As you pointed out, the same bug problem occurred in page caching and a PR ( fix: change strategy for route caching #10248 ) have solved it by putting a string key instead of using route.component.
  3. Astro Integration, like mine, may want to generate multiple collections (created by the users) based on a common template. I think my project proves that there is a valid use case. The alternative is to handle everything inside a unique entrypoint like Starlight does, but it's a real gas factory and seems counterintuitive to me.

I just opened a PR #10625 , where I'll try to implement a fix inspired by #10248 .

@Fryuni
Copy link
Member

Fryuni commented Mar 31, 2024

I sent a commit on a branch of mine with the integration test reproducing this issue to validate it. If you haven't got to testing maybe it will help you: ccc33c3

@goulvenclech
Copy link
Contributor Author

Thanks @Fryuni , I'll use this!

@ematipico
Copy link
Member

To support multiple routes using the same entrypoint, this would need to be an array with all the routes with the same route.component. Changing this will also require changing the internals.routePerComponent to an array and every place that uses it. This affects quite a few places so it is not a small change, but TS should be able to guide you through it.

I went down this road before, when I was implementing the fallback system for i18n routing. The logic is exactly the same: to allow a component to generate multiple routes.

I didn't pursue this solution because:

  • it could be a breaking change for integrations because we pass down such information to them (unless we want to make this backwards compatible, but it requires way more work)
  • it's not a bug fix, but a new feature
  • it requires some discussion around expectations
  • it's more complex than you expect

If you're OK, I am going to remove the bug label for the time being.

@ematipico ematipico added needs discussion Issue needs to be discussed and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Apr 3, 2024
@Fryuni
Copy link
Member

Fryuni commented Apr 4, 2024

The minor bug is more for the inconsistency between dev and prod. That I consider a bug.

Blocking it on dev would be fixing the bug, making it work for prod builds would be a new feature.

@ematipico
Copy link
Member

I looked at the PR that OP made and they were trying to implement the feature. What's the best course of action?

@Fryuni
Copy link
Member

Fryuni commented Apr 4, 2024

I am personally in favor of this new feature because I'm already thinking of the fun ways I can use it to push the limits, so it's not an unbiased take.

But I think it is better to get consistent behavior first. Reaching a decision regarding this new feature and then reaching a good implementation will take some considerable time.

@goulvenclech
Copy link
Contributor Author

goulvenclech commented Apr 4, 2024

@ematipico I'm not in favour of removing the "bug" tag—for the same reason as @Fryuni —but of course, this should need some discussion. I would love to have your opinion on the three arguments I raised above -> #10622 (comment)

This week I have a lot of work, that's why I didn't continue this PR, but I'll come back to it soon. Here is my plan in chronological order:

  1. Fix the bug and get the behavior I want, as a POC
  2. Make sure this hasn't broken anything (I don't plan to change the tests apart from the one I added), especially in the public API for integrations
  3. Clean up after myself, including improving naming and documentation.

Since this bug break my integration, I'm pretty motivated to solve it 😛 and it's not a problem if it takes some considerable time.

@ematipico
Copy link
Member

So what should be the fix?

@goulvenclech
Copy link
Contributor Author

goulvenclech commented Apr 4, 2024

@ematipico As I said before here and in the PR, for me, we should never get the pages by their component, since 1. there is no reason why two pages can't have the same entrypoint 2. there is clear use cases where integration could want to have the same entrypoint for multiple pages.

The fix should be either :
a - using a key, like you did in the cache part ( #10248 )
b - using the path or the pattern, because I don't see how two pages could use the same path/pattern
c - change the record for a Set or an Array

I'm currently doing my PR following the solution a, because it's coherent with your PR (that has been merged before). But I'm in favour of the solution b or c, but keeping in mind that the solution C could break more things along the way.

@ematipico
Copy link
Member

Is there a place in the docs where this topic isn't explained very well or lives in some room of interpretation?

I ask this because, as far as I know, it's already possible for a component to emit multiple paths, and that's done using the spread route e.g. [...dynamic]

@goulvenclech
Copy link
Contributor Author

@ematipico I'm sorry, but I don't think you understood the bug. What can I clarify in my or @Fryuni 's messages to help you?

The problem is not about spread route. Nor emitting multiple paths for a given entrypoint. @Fryuni & myself knows that this feature works, since we use it daily.

The problem is about the integration API & and how we get pages (by components/entrypoints) in the build process, making it impossible for the function injectRoutes() (only in prod) to inject multiples (static or spread) routes with the same entrypoint.

This problem is for both spread routes and static routes (see the test & my use case), is a bug because differs from dev (correct behaviour) and production (merged routes), no error nor warning are raised, and this limitation is not documented.

@goulvenclech
Copy link
Contributor Author

goulvenclech commented Apr 4, 2024

To clarify the bug, I have made another (not so) minimal repro -> https://stackblitz.com/edit/github-l3a17a?file=src%2F%5B...slug%5D.astro

This one have spread routes like :

          injectRoute({
            pattern: `/blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });

In dev, everything work as intended, with 5 pages generated with the correct URLs :

/ <- src/pages/index.astro
/blog <- src/blog.astro
/another-blog <- src/blog.astro
/blog/test <- src/[...slug].astro
/another-blog/another-test  <- src/[...slug].astro

Here how it's merged in build :

generating static routes 
12:06:19 ▶ src/blog.astro
12:06:19   └─ /blog/index.html (+24ms)
12:06:19 ▶ src/[...slug].astro
12:06:19   ├─ /blog/test/index.html (+29ms)
12:06:19   └─ /blog/another-test/index.html (+6ms)
12:06:19 ▶ src/pages/index.astro
12:06:19   └─ /index.html (+1ms)
12:06:19 ✓ Completed in 176ms.

We loose one the blog's index. And one article is merged into the wrong URL.

@goulvenclech
Copy link
Contributor Author

Just updated my initial post with this second example.

@ematipico ematipico added pkg: astro Related to the core `astro` package (scope) - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs discussion Issue needs to be discussed labels Apr 4, 2024
@ematipico
Copy link
Member

Thank you, @goulvenclech, for clarifying the use case and apologies for juggling too much on this issue.

It would be great if we could also follow up with a docs PR to also show the usage we are going to fix, as proof of user expectations of the API: https://docs.astro.build/en/reference/integrations-reference/#injectroute-option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants