Refactor/types #11715

Princesseuh · 2024-08-15T03:16:32Z

Changes

Back in 1997, when Astro was just getting started, someone created an innocent @types/astro.ts file, mostly intended for small types used throughout the codebase and types for untyped modules. With time, this file grew with a mix of internal, public, types from dependencies, unused types etc, to a total of 3000 lines of types.

This PR attempt to untangle that file, only keeping public what should actually be public and going away from the easily confused with DefinitelyTyped's stuff @types to a normal folder called.. types. I also took this opportunity to split the types into multiple files, just so we don't have to scroll 3000 lines of types to find something. No big thoughts went into the splits, just put things where it seemed they'd belong.

Throughout this journey, I discovered that more types than I thought were exposed as part of the public API, perhaps as a mistake.

Testing

It builds! We don't have a lot of tests regarding types, but the little we do have do pass too. Smoke tests will fail because of docs docgen script, which need to get adapted to this new structure.

Docs

withastro/docs#9109

changeset-bot · 2024-08-15T03:16:37Z

🦋 Changeset detected

Latest commit: da94ba7

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

github-actions

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

Princesseuh · 2024-08-15T03:19:43Z

packages/astro/src/content/loaders/glob.ts

@@ -131,7 +131,7 @@ export function glob(globOptions: GlobOptions): Loader {
 				if (entryType.getRenderFunction) {
 					let render = renderFunctionByContentType.get(entryType);
 					if (!render) {
-						render = await entryType.getRenderFunction(settings);
+						render = await entryType.getRenderFunction(settings.config);


This is the only runtime change I did, the way it was setup, it was exposing AstroSettings publicly, which isn't in line with what we typically do

Princesseuh · 2024-08-15T03:26:45Z

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

@@ -0,0 +1,202 @@
+// TODO: Should the types here really be public?


A lot of things in this file feel like they shouldn't be public, but end up being for various reasons. The most concerning of which was RouteData, which is exposed on integrations hooks.

A lot of the SSR*** ones are also spooky, but they're used by renderers which is not too bad

Princesseuh · 2024-08-15T03:33:31Z

packages/astro/client.d.ts

-// eslint-disable-next-line  @typescript-eslint/no-namespace
-declare namespace App {
-	// eslint-disable-next-line  @typescript-eslint/no-empty-interface
-	export interface Locals {}
-}
-


This is unnecessary, the default definition is already that. However, it was done in a .d.ts file, which don't get sent to dist so it never worked.

Princesseuh · 2024-08-15T03:33:46Z

packages/astro/client.d.ts

@@ -191,7 +185,6 @@ declare module '*.md' {
 		file,
 		url,
 		getHeadings,
-		getHeaders,


That method hasn't existed in a while.

Princesseuh · 2024-08-15T03:37:10Z

packages/astro/src/jsx/renderer.ts

-	jsxImportSource: 'astro',
-	jsxTransformOptions,


Those options don't seem to exist anymore? There was a deprecated message on both of them.

Yeah I'm planning to remove the entire JSX handling here in a separate PR once I refactor the MDX intgeration stuff. Removing it here now is fine though.

Princesseuh · 2024-08-15T03:37:56Z

packages/astro/src/runtime/server/endpoint.ts


 /** Renders an endpoint request to completion, returning the body. */
 export async function renderEndpoint(
-	mod: EndpointHandler,
+	mod: {
+		[method: string]: APIRoute;


Type was fully internal to this line, figured there was no point to it being a separate type.

Princesseuh · 2024-08-15T03:45:48Z

packages/astro/src/integrations/hooks.ts

@@ -198,10 +197,6 @@ export async function runHookConfigSetup({
 				addWatchFile: (path) => {
 					updatedSettings.watchFiles.push(path instanceof URL ? fileURLToPath(path) : path);
 				},
-				addDevOverlayPlugin: (entrypoint) => {
-					// TODO add a deprecation warning in Astro 5.


The TODO here is wrong, this API was unsupported and undocumented before the toolbar actually became stable, Astro 5 is prime time for fully removing it (the only app that uses it, Sentry, only uses it as a fallback)

bluwy

I hope you used some sort of refactoring tool that made this journey less painful 😅 But anyways I stepped through each of them and it looks fine to me.

Should we also update

astro/CONTRIBUTING.md

Line 292 in 5e928f5

- `@types/`: TypeScript types. These are centralized to cut down on circular dependencies
In what case would we use the ts files in types/*.ts (not the public folder)? Should we place private shared types in types/astro.ts, or should we lean towards putting types inside each domains' file/folder instead? (for example, there's a lot of types.ts file.

Princesseuh · 2024-08-15T09:03:32Z

Yea, should definitely update that!

I'm not sure! I think that it's fine for some very specific types that are used everywhere perhaps, but overall it seems like over time we've done more and more random types files closer to where the feature live. Do you have a preference?

bluwy · 2024-08-15T09:26:27Z

I like to group them by domain, so the types.ts file is fine for me, or if it's inlined to the file that uses it first. If we're ok with this pattern too, it would be nice to put a comment in types/astro.ts that describes when to use or not use it. Maybe for types that really encompasses Astro, like AstroSettings is fine to be there though.

Princesseuh · 2024-08-15T09:33:00Z

I'll add some documentation all around

Princesseuh added 2 commits August 14, 2024 17:26

refactor: types

e179369

feat: refactor types

f286a0b

Princesseuh changed the base branch from main to next August 15, 2024 03:16

github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) semver: major Change triggers a `major` release labels Aug 15, 2024

github-actions bot requested changes Aug 15, 2024

View reviewed changes

Princesseuh commented Aug 15, 2024

View reviewed changes

chore: organize imports

9279d56

github-actions bot removed the pkg: integration Related to any renderer integration (scope) label Aug 15, 2024

Princesseuh commented Aug 15, 2024

View reviewed changes

fix: remaining references

5e928f5

github-actions bot added 🚨 action Modifies GitHub Actions pkg: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review labels Aug 15, 2024

Princesseuh marked this pull request as ready for review August 15, 2024 03:56

bluwy approved these changes Aug 15, 2024

View reviewed changes

Princesseuh added 3 commits August 15, 2024 16:12

docs: types

0a5a2d9

refactor: small changes

3d0cb57

chore: changeset

b931b02

yanthomasdev mentioned this pull request Aug 15, 2024

[5.0] Update docgen script for new path to types withastro/docs#9109

Closed

Merge branch 'next' into refactor/types

da94ba7

ematipico approved these changes Aug 16, 2024

View reviewed changes

Princesseuh merged commit d74617c into next Aug 16, 2024
12 of 14 checks passed

Princesseuh deleted the refactor/types branch August 16, 2024 12:07

astrobot-houston mentioned this pull request Aug 16, 2024

[ci] release (alpha) #11627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/types #11715

Refactor/types #11715

Princesseuh commented Aug 15, 2024 •

edited

Loading

changeset-bot bot commented Aug 15, 2024 •

edited

Loading

github-actions bot left a comment

Princesseuh Aug 15, 2024

Princesseuh Aug 15, 2024 •

edited

Loading

Princesseuh Aug 15, 2024

Princesseuh Aug 15, 2024

Princesseuh Aug 15, 2024

bluwy Aug 15, 2024

Princesseuh Aug 15, 2024

Princesseuh Aug 15, 2024 •

edited

Loading

bluwy left a comment

Princesseuh commented Aug 15, 2024

bluwy commented Aug 15, 2024

Princesseuh commented Aug 15, 2024

		@@ -0,0 +1,202 @@
		// TODO: Should the types here really be public?

Refactor/types #11715

Refactor/types #11715

Conversation

Princesseuh commented Aug 15, 2024 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Aug 15, 2024 • edited Loading

🦋 Changeset detected

github-actions bot left a comment

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024

Choose a reason for hiding this comment

bluwy Aug 15, 2024

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024

Choose a reason for hiding this comment

Princesseuh Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

bluwy left a comment

Choose a reason for hiding this comment

Princesseuh commented Aug 15, 2024

bluwy commented Aug 15, 2024

Princesseuh commented Aug 15, 2024

Princesseuh commented Aug 15, 2024 •

edited

Loading

changeset-bot bot commented Aug 15, 2024 •

edited

Loading

Princesseuh Aug 15, 2024 •

edited

Loading

Princesseuh Aug 15, 2024 •

edited

Loading