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

feat: injectDts and codegenDir #9952

Closed

Conversation

florian-lefebvre
Copy link
Member

@florian-lefebvre florian-lefebvre commented Feb 2, 2024

Changes

  • Adds a new internal setting codegenDir
  • Adds a new integration option injectDts
  • Adds a new integration option codegenDir
  • Rework astro content to use the new internal injectDts
  • Reworks a script to use tsconfck

This PR initially meant to tackle withastro/roadmap#826 but it proved too difficult for me. I decided to reduce the PR content so it does not handle the autogenerated tsconfig. We'll have to discuss this later on, because it's really tricky!

Codegen seems to become a quite common usecase for integrations (for example: @astrojs/db, astro-env, @keystatic/astro, astro-typed-api...). Let's look at 2 cases:

Adding files to the .astro dir

Before this PR, integrations had to make sure themselves that the .astro dir existed before adding files to it, and also had to compute the path from the root. Now, the .astro directory is always created before and its path can conveniently be accessed as codegenDir (URL). This change is also convenient for Astro core as it allows any core feature to access the codegenDir (through settings), not only content related code.

Adding types

Before this PR, integrations authors had to create a .d.ts file inside the .astro directory, then update src/env.d.ts to add a relative reference to this file. Now, .astro/types.d.ts is always created and creates references to other .d.ts files so that there can only be one reference in src/env.d.ts. For now, this file is called types.d.ts for backward compatibility reasons (that's the former filename of .astro/content.d.ts) but astro.d.ts is reserved as it seems to be a nicer convention. That can be an Astro 5 breaking change. Also worth noting that in Astro 5, this allows us to remove the astro/client reference in env.d.ts as it's already present in .astro/types.d.ts.

Testing

  • Existing tests should pass
  • New tests
    • codegenDir created before integrations
    • types.d.ts content
    • injectDts (internal + integration)

Docs

Copy link

changeset-bot bot commented Feb 2, 2024

🦋 Changeset detected

Latest commit: faa8f43

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 github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Feb 2, 2024
@github-actions github-actions bot added pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) labels Feb 8, 2024
packages/astro/src/config/types.ts Outdated Show resolved Hide resolved
) {
logger.warn(
null,
`The integration ${integration.name} is overriding a core dts: ${dts.filename}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was perfectly ok with the name until I read this error. Should we call them "ambient declarations", which is what they are, instead of "dts", which is a simplification of their extension?

I imagine a user would not know what a "core dts" is. Searching for "core dts" or "dts file" also doesn't help since there is a .dts extension. Searching for "ambient declarations" on the other hand immediately directs to TypeScript.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to any naming! All the messages are really WIP and can be improved a lot for sure! (i'll focus on that in a 2nd phase, with documentation) For reference, @nuxt/kit has a addTypeTemplate utility

@florian-lefebvre florian-lefebvre added this to the 4.5 milestone Feb 27, 2024
@florian-lefebvre florian-lefebvre marked this pull request as ready for review February 27, 2024 17:20
scripts/smoke/check.js Outdated Show resolved Hide resolved
Comment on lines +16 to +17
"astro:config:setup": ({ codegenDir }) => {
const filePath = new URL("./data.json", codegenDir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that we expose codegenDir here? I think we'd want to "own" this, so not allowing or encouraging integrations to put files here. In the future if Astro wants to write a new file/folder here it could conflict with other integrations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I just realized that injectDts writes an exact <name>.d.ts file inside .astro, so this would already be an issue 🤔 I thought the types are all concat into a single .astro/types.d.ts. I'm not sure if that's also possible.

Copy link
Member Author

@florian-lefebvre florian-lefebvre Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the codegenDir being exposed to integrations: I think it's useful to have it because integrations are going to use it anyway (eg. @astrojs/db or @keystatic/astro). If we want to avoid conflicts, I think we can define some conventions to make all of this safe. For example, we could have this structure:

.astro
	astro.d.ts // reference any other registered types
	astro
		content.d.ts
		env.d.ts // for example for the types rfc
	integrations
		whatever
			some-types.d.ts

If we go that way, that would make things safer by ensuring each integration has its own space, not conflicting with core. In that case, codegenDir would be different for any integration, basically ${root}/.astro/integrations/${integrationName} and injectDts would then inject in this folder.


About injectDts and exact .d.ts files, this is actually on purpose. I think it's better mainly for debuggability. Like, I wouldn't want to debug my injected types, lost somewhere after 2k lines of astro content and before 5 other integrations. Also, it can be unsafe to have them all in one file if there are some name conflicts, eg.

// injected by integration a
type Test = 'a'

declare module 'virtual:a' {
	export const a: Test;
}

// injected by integration b
type Test = 'b'

declare module 'virtual:b' {
	export const b: Test;
}

Test would conflict here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply, I think if we want to have codegenDir be .astro, then perhaps we should rename it to tempDir/astroDir/tempAstroDir/dotAstroDir/cacheDir since it has multiple uses. Otherwise I think reading config.root would also work.

I don't think we need to take measures to avoid name conflicts for now, but perhaps putting all of the types from injectDts inside .astro/types/* should be enough, and document that integrations should use a name unique to its integration.

Good point about the injectDts conflict, I didn't thought of that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cacheDir is already taken (see docs). tbh I feel like internally we could call this dotAstroDir and for integrations still codegenDir because if we head into the direction of my proposal, such URL will not technically point to .astro. I am going to post a new comment with the updated proposal based on all the reviews

.changeset/tender-pens-sell.md Show resolved Hide resolved
packages/astro/src/config/types.ts Show resolved Hide resolved
packages/astro/src/core/build/index.ts Show resolved Hide resolved
packages/astro/src/config/types.ts Show resolved Hide resolved
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just putting a "Docs Block" here because there still seems to be a lot of discussion going on in the changeset, which means I'm not sure that stuff has been resolved here yet. Ping me when this content is agreed-upon and finalized by everyone, then I'll review this with the docs PR together to make sure they align!

@florian-lefebvre florian-lefebvre removed this from the 4.5.0 milestone Mar 3, 2024
@florian-lefebvre
Copy link
Member Author

florian-lefebvre commented Mar 4, 2024

Thanks everyone for the amazing feedback! Based on all those, here is a proposal that I think addresses your concerns and reviews.

.astro structure rework

Currently, the .astro dir contains only types.d.ts, injected by the content plugin. With this PR, here is how the structure of this folder would look:

.astro/
	types.d.ts // only references to other .d.ts. We keep this name for backward compatibility,
				  can be changed to something like astro.d.ts in Astro 5
	astro/
		content.d.ts // formerly types.d.ts
	integrations/
		<integration_name>/
			<dts_name>.d.ts

Internal changes

  • Internally, a new dotAstroDir (URL) is accessible from the settings to access .astro in an unified way
  • Internally, a new injectTypes takes a name (enforced by a regex to only allow letters, dashes and underscores) and creates a .astro/astro/<name>.d.ts

astro sync changes

This PR means that the sync command is now not only for core types, but for codegen in general (even from integrations). It means it's more tied to astro:config:setup so syncInternal probably needs to be called in more places.

syncInternal now creates the .astro/types.d.ts with all the references to any other created .d.ts file.

Integrations API changes for astro:config;setup

  • New property codegenDir (URL), available at .astro/integrations/<integration_name>/. This means we probably need to normalize the name somehow to avoid creating folders. Can be probably enforced by replacing problematic characters by like _ or something
  • New function injectTypes. Basically the same as core but inject types at codegenDir.

Let me know what you think!

@florian-lefebvre
Copy link
Member Author

florian-lefebvre commented Apr 4, 2024

I'm closing this PR in favor of a proper RFC (future, once defineIntegration is finally part of core), I didn't think this PR would get so complex

@florian-lefebvre florian-lefebvre deleted the feat/ts-improvements branch April 4, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants