feat(next): astro:routes:resolved #12329

florian-lefebvre · 2024-10-28T19:52:23Z

Changes

Addresses Give integrations access to routes earlier roadmap#998
Introduces a new astro:routes:resolved hook, that runs before astro:config:done and on any route change on dev
In refactor(next): send IntegrationRouteData to integrations #11864, we talked about bigger refactorings but left the shape similar for backward compatibility. With this PR, we can do whatever we want with the shape!

Testing

Adds a test for dev

Docs

Will write docs after the PR is approved

changeset-bot · 2024-10-28T19:52:28Z

🦋 Changeset detected

Latest commit: 5576af9

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

florian-lefebvre · 2024-10-29T12:55:14Z

packages/astro/src/container/index.ts

@@ -544,6 +544,7 @@ export class experimental_AstroContainer {
 			type,
 			fallbackRoutes: [],
 			isIndex: false,
+			origin: 'user',


I'm not sure what we should use here

github-actions

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Fryuni · 2024-10-29T23:18:12Z

packages/astro/src/core/routing/astro-designed-error-pages.ts

@@ -15,6 +15,7 @@ export const DEFAULT_404_ROUTE: RouteData = {
 	route: '/404',
 	fallbackRoutes: [],
 	isIndex: false,
+	origin: 'core',


Although we use "core" to refer to the astro package, is this as clear to users of the API as it is to us? Maybe origin: 'astro' would be better for users.

packages/astro/src/types/public/integrations.ts

Fryuni · 2024-10-29T23:27:30Z

packages/astro/src/types/public/integrations.ts

+	/**
+	 * Dynamic and spread route params
+	 * ex. "/pages/[lang]/[...slug].astro" will output the params ['lang', '...slug']
+	 */
+	params: RouteData['params'];


I'll miss the generate function from IntegrationRouteData to generate URLs for a given route. Integrations will have to construct their own URL generator function based on the pattern and params which seems somewhat unnecessary since that function already exists in Astro and just isn't exposed.

It is not a blocker since it can be done in user-land

I've included the minimum properties, but it's the opportunity to rethink this api tbh! So feel free to suggest things and we'll talk about it at the standup

I think it makes sense to keep generate

The only field used on official integrations/adapters that can't be recomputed on the consumer is type to indicate whether the route is an endpoint, page, redirect, or fallback.

pathname is now pattern, so it is just a rename

segments can be computed from pattern

generate can be created from segments

So I think at least type should be present.

I wonder if segments and generate should be properties on the object, or functions exported by Astro where you can pass pattern/segments

packages/astro/src/vite-plugin-astro-server/plugin.ts

Fryuni · 2024-10-29T23:55:09Z

packages/astro/src/types/public/integrations.ts

@@ -225,13 +217,18 @@ export interface BaseIntegrationHooks {
 	'astro:build:done': (options: {
 		pages: { pathname: string }[];
 		dir: URL;
+		/** @deprecated Use `routes` from `astro:routes:resolved` instead */


If we intend to remove this, we'll need to think more about how to provide the existing functionality elsewhere. Fields not exposed on the new hook are relied upon by integrations and adapters, including official ones:

Sitemap relies on type, pathname, and generate

Most adapters rely on type to emit redirects efficiently in their specific format

The Cloudflare adapter relies on the segments

Indeed! Hence the need to think about all the properties we should include, it doesn't need to be a 1:1

distURL shouldn't be part of the new type, however it should be provided in the hook build:done;

Perhaps, it makes sense to evaluate a new type for the build:done hook, and not pass certain types, such as params, segments, generate.

Then we can deprecate properties on build:done routes, instead of the whole routes list

Then we can deprecate properties on build:done routes, instead of the whole routes list

Yeah that sounds like a good thing

ematipico · 2024-10-30T08:38:14Z

@florian-lefebvre

I read the previous PR, and I couldn't find anything regarding origin. Could you please provide a reasoning about why it was added? Maybe in the description PR

ematipico · 2024-10-30T09:23:32Z

packages/astro/src/types/public/internal.ts

+	/**
+	 * Whether the route comes from Astro core, an integration or the user's project
+	 */
+	origin: 'core' | 'integration' | 'user';


Some alternatives:

internal: coming from Astro

external: coming from integrations outside Astro core

project: coming from a user project

origin was suggested by Luiz, see withastro/roadmap#998 (comment)

My suggestion was regarding the variants, not the name origin.

core -> internal (we don't usually use the term core when "talking" to users)

integration -> external

user -> project (users think about project, not "user". A project might belong to a team, multiple users)

packages/astro/src/types/public/internal.ts

.changeset/giant-ravens-look.md

packages/astro/src/types/public/integrations.ts

ematipico · 2024-10-30T11:12:16Z

packages/astro/src/types/public/integrations.ts

+	/**
+	 *  Source component URL
+	 */
+	entrypoint: RouteData['component'];


Why do we change the internal naming? Wouldn't it create fragmentation? Also, we shifted into a pattern where "entrypoint" is URL or string, where the string is an absolute file path. In this case, these are string, and they are relative paths. I think we should keep component, so they are not misunderstood like the other "entrypoints" we have in other APIs.

I proposed changing the "component" name for the new public API because the value here is not necessarily a component. This is already true of RouteData['component'], the name is misleading.

I'm not sure whether entrypoint is the best name either. It can be:

A relative path when the route is a redirect

The absolute path to an Astro component when the route is a page

The import path (before resolving dependencies) of an Astro component when the route is a page

The absolute path to a JS file when the route is an API

The import path (before resolving dependencies) of a JS file when the route is an API

It is quite an overloaded field, so I think it needs a more generic name than "component".

Some options and my thoughts about them:

source: would be misleading for redirect since it is where it redirects to and not from

definition: I don't think it really fits with redirect

implementation: also doesn't fit with redirect

target: seems weird to say an Astro component is the target of a page but could work

reference: in the sense of a "reference implementation" for the page or API and the reference link for the redirect

What about origin or creator?

packages/astro/src/types/public/integrations.ts

ematipico · 2024-10-30T11:18:19Z

packages/astro/src/types/public/integrations.ts

@@ -225,13 +217,18 @@ export interface BaseIntegrationHooks {
 	'astro:build:done': (options: {
 		pages: { pathname: string }[];
 		dir: URL;
+		/** @deprecated Use `routes` from `astro:routes:resolved` instead */


distURL shouldn't be part of the new type, however it should be provided in the hook build:done;

Perhaps, it makes sense to evaluate a new type for the build:done hook, and not pass certain types, such as params, segments, generate.

ematipico · 2024-10-30T11:21:42Z

packages/astro/src/types/public/integrations.ts

+	/**
+	 * Dynamic and spread route params
+	 * ex. "/pages/[lang]/[...slug].astro" will output the params ['lang', '...slug']
+	 */
+	params: RouteData['params'];


I think it makes sense to keep generate

packages/astro/src/vite-plugin-astro-server/plugin.ts

Co-authored-by: Luiz Ferraz <[email protected]>

florian-lefebvre · 2024-11-13T15:57:05Z

packages/astro/src/types/public/integrations.ts

 	 */
 	redirectRoute?: IntegrationRouteData;
 };
+
+export interface IntegrationResolvedRoute


The discussion was marked as resolved because of commits, but we need to figure out the renaming of some fields

florian-lefebvre · 2024-11-13T16:29:26Z

packages/astro/test/units/integrations/api.test.js

+					);
+					await new Promise((r) => setTimeout(r, 100));
+
+					assert.deepEqual(


This is failing but I can't figure out why. Looks like the partial update of the manifest is not working as expected

feat: work on astro:routes:resolved

621d2d2

github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr labels Oct 28, 2024

florian-lefebvre added 3 commits October 29, 2024 13:39

feat: work on jsdoc

fbb04e9

feat: support origin

98e77c9

misc

96f560b

florian-lefebvre commented Oct 29, 2024

View reviewed changes

florian-lefebvre added 2 commits October 29, 2024 14:57

feat: add test

65f5b60

feat: add changeset

75a84dc

github-actions bot added the semver: minor Change triggers a `minor` release label Oct 29, 2024

github-actions bot requested changes Oct 29, 2024

View reviewed changes

florian-lefebvre marked this pull request as ready for review October 29, 2024 14:06

Fryuni requested changes Oct 29, 2024

View reviewed changes

ematipico reviewed Oct 30, 2024

View reviewed changes

matthewp approved these changes Oct 31, 2024

View reviewed changes

florian-lefebvre and others added 8 commits November 1, 2024 14:08

Update packages/astro/src/types/public/integrations.ts

3a3b083

Co-authored-by: Luiz Ferraz <[email protected]>

feat: rename property

ec16abf

Merge branch 'next' into feat/routes-resolved

ce0fc54

feat: handle partial manifest updates in dev

7e26fa4

Merge branch 'next' into feat/routes-resolved

319b405

feat: include more properties and rename origin things

db92a98

feat: update origin

fc4e42c

feat: deprecate properties instead of whole object

ebb12f9

florian-lefebvre commented Nov 13, 2024

View reviewed changes

florian-lefebvre added 3 commits November 13, 2024 16:58

Update packages/astro/src/types/public/internal.ts

b377e0f

feat: update test

33b1165

feat: work on test

5576af9

florian-lefebvre commented Nov 13, 2024

View reviewed changes

florian-lefebvre requested review from Fryuni and ematipico November 13, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(next): astro:routes:resolved #12329

feat(next): astro:routes:resolved #12329

florian-lefebvre commented Oct 28, 2024 •

edited

Loading

changeset-bot bot commented Oct 28, 2024 •

edited

Loading

florian-lefebvre Oct 29, 2024

github-actions bot left a comment

Fryuni Oct 29, 2024

Fryuni Oct 29, 2024

florian-lefebvre Oct 30, 2024

ematipico Oct 30, 2024

Fryuni Oct 31, 2024

florian-lefebvre Nov 1, 2024

Fryuni Oct 29, 2024

florian-lefebvre Oct 30, 2024

ematipico Oct 30, 2024

florian-lefebvre Nov 1, 2024

ematipico Nov 1, 2024

ematipico commented Oct 30, 2024

ematipico Oct 30, 2024

florian-lefebvre Nov 1, 2024

ematipico Nov 1, 2024 •

edited

Loading

ematipico Oct 30, 2024

Fryuni Oct 30, 2024

ematipico Nov 1, 2024

florian-lefebvre Nov 13, 2024

ematipico Oct 30, 2024

ematipico Oct 30, 2024

florian-lefebvre Nov 13, 2024

florian-lefebvre Nov 13, 2024

feat(next): astro:routes:resolved #12329

Are you sure you want to change the base?

feat(next): astro:routes:resolved #12329

Conversation

florian-lefebvre commented Oct 28, 2024 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Oct 28, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florian-lefebvre commented Oct 28, 2024 •

edited

Loading

changeset-bot bot commented Oct 28, 2024 •

edited

Loading

ematipico Nov 1, 2024 •

edited

Loading