fix(page-data): add key to allPages

[goulvenclech]

Current changes

As now :
Fixes #10622 , by changing the logic of "getting all pages by their component" by "getting all pages by a key" in the build part. Inspired by how the PR #10248 implemented the same solution in the route caching part. Also, remove a bunch of generators (verbose and inneficients).

Since this doesn't change anything in the public API, I guess it's a patch? But because I did deep changes in the build process, I'll understand if you prefer this to be a minor. I leave that to reviewers 🙂

For Astro 5 :
internals has a new function getPageDatasWithPublicKey(). It only exist to avoid breaking change in the integration API, but maybe we should remove it at the next major update?

Testing

@Fryuni provided me with an integration test, see packages/astro/test/reuse-injected-entrypoint.test.js. I don't plan to change the tests apart from that one.

Docs

Should this be documented? I feel like this is just the expected behaviour.

[changeset-bot]

🦋 Changeset detected

Latest commit: 27395f4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[goulvenclech]

Okay @Fryuni @Princesseuh , the good news is: I fixed the key problem in allPages without breaking existing tests.

The bad news is: it didn't solve my problem. So I guess the problem runs deeper... 🕵🏻‍♂️

[goulvenclech]

Current suspect : trackPageData set internals.pagesByComponents. I'm trying to replace it by internals.pagesByKeys (since pageData has the key now)

[goulvenclech]

On an unrelated (but kind of related) note:
I find some naming in this part of the code REALLY confusing. For example, for allPages et internal.pagesByComponents, the key "component" is sometimes called "path", "pageName", "page" or "component" indistinctively.
Also some naming like pageData2 looks kind of WIP. Same for output.output or route.route.

[goulvenclech]

There is still a pagesByKeys.get() to fix in packages/astro/src/core/build/plugins/plugin-pages.ts, but this bad boy will require a bit more work...

[goulvenclech]

Still a lot of work to get rid of the "getting all pages by their component" logic everywhere. But I'm still thinking : should this use a key (like the PR mentioned above), or should this use path ? (Could be more useful + two routes can't have the same path).

[goulvenclech]

Struggling to understand internals.entrySpecifierToBundleMap & virtualModuleName/virtualModulePageName and their usage... If someone can provide me some help / informations / context, it could help me a lot.

[Fryuni]

internals.entrySpecifierToBundleMap is a mapping from the ID of the virtual module that represents an entrypoint to the Astro application (aka, what should be imported to render a route) to the name of the file on disk, part of the bundle, that exports that entrypoint. It is populated at the final stage of a Vite build before Astro starts pre-rendering pages.

It is used for the rendering pipeline once it knows which routes should be rendered during the build, it needs to know from which file to import the renderer that needs to run.

---

virtualModuleName/virtualModulePageName is the name of a Virtual Module that will be populated with compiled information from an Astro file. If it were the name of a regular file, Rollup (the underlying bundler used by Vite) would try to read the file, and other plugins might attempt to process it as if it were a regular file. It isn't a regular file, it is used for things like:

• The styles extracted from a .astro component.
• The client-side scripts from a .astro component that need to be bundled only for the client (contrary to the code for the Astro component itself that is only for the server).

[Fryuni]

I find some naming in this part of the code REALLY confusing. For example, for allPages et internal.pagesByComponents, the key "component" is sometimes called "path", "pageName", "page" or "component" indistinctively.

The value from component is called different things in different places because it means different things in those different places.

A "Route", conceptually in Astro, can be either:

• An entry from the filesystem routing in the project
• An injected route from an integration
• A redirect

When it comes from the filesystem or from an integration, the "component" is the entrypoint defining the route. It may be called page/pageName in some cases because it is the name under which the pages built from that route will be displayed.

When it is a redirect, the "component" really is a path. It is the target of the redirect.

---

That being confusing is great feedback, thank you! We have been talking recently exactly about where things could be confusing for new contributors.

[matthewp]

Great example of some of the more complicated internals and how they sort of just grew and never got a proper refactor / rethinking. We're unlikely to do that, but renaming properties to make them make more sense, and adding inline documentation to explain what they are for, is something that could be done more easily.

[goulvenclech]

@Fryuni Thanks a lot for the first review. 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.

For the key I'm wondering : I've choose this key pattern because it's the same one as @ematipico in #10248 . But if we want to do something different, maybe we could just use the path (getPageByPath) or the pattern (getPageByPattern) since, unlike the component, it doesn't seem possible to have two pages with the same path?

Or maybe we shouldn't use a Record here ? And I should change that to a Set or an Array ? But I'm afraid this will break even more things.

[goulvenclech]

@matthewp I won't do any refactor/ rethinking since I'm not in the Astro team and I would like this to be a minor fix. But I'll simplify some over-engineered parts (I already deleted some generators), add documentation, and change some naming. Here is my course of action:

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.

[Fryuni]

it doesn't seem possible to have two pages with the same path?

It is. Two integrations can define the same dynamic route, like [slug] and render different slugs each. That is perfectly fine.

[Fryuni]

Looking GREAT to me!

I left just a naming suggestion that I think would improve clarity; I got a bit confused about what it really meant for a file to be in that set.

[bluwy]

Awesome work! The logic all looks good to me.

[sarah11918]

Just reviewing for docs, and have some tiny language edits for the changeset!

[goulvenclech]

Thanks @sarah11918 !

BTW, I just remembered that @ematipico suggested adding a usage example in the docs (See in the issue -> #10622 (comment) ).

Is this still relevant? Should I take care of this, and what should I have in my doc PR?

[sarah11918]

Hi @goulvenclech ! I checked out the docs in question, and I think we only need a docs PR if something in that section/instruction now works differently. As you wrote in your issue, this may simply have been an unexpected limitation, and people just assumed it would have already worked this way. I will also tag in @Fryuni since he understands the underlying feature better than I do! 😄

Is anything on the Integrations API reference page now incorrect or misleading as a result of your feature?

[goulvenclech]

Is anything on the Integrations API reference page now incorrect or misleading as a result of your feature?

Not incorrect nor misleading, the doc didn't talk about since it wasn't decided (different behavior between dev and prod). I think there are two options:

1. In the Astro Integration API documentation at the "InjectRoute" section, add a paragraph clarifying that we can use the same Astro component as an entrypoint for several pages. Illustrate with the use case -> inject several blogs (using the same layout component blog.astro and [...entry].astro) for several paths provided by the user.

2. Acknowledge that this would fill the documentation with a marginal use case, and that it is not worth it.

Your call 🙂 both suit me, and I can quickly do the PR doc.

[goulvenclech]

@sarah11918 Just commited your changeset. Noted for "when building an Astro integration" as an use case, makes sense. And noted for "marginal use case so no doc for now", I think this is the expected behavior anyway.

Thanks for your time 🙂

[sarah11918]

Approving on behalf of docs! Thank you @goulvenclech ! 🙌

Merge day