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

[Content collections] Move generated types to .astro directory #5786

Merged
merged 19 commits into from
Jan 10, 2023

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Jan 6, 2023

Changes

  • Moves generated types from src/content/types.generated.d.ts -> .astro/types.d.ts
  • Generate a src/env.d.ts if none exists with the astro/client and .astro types
  • Add .astro reference path to src/env.d.ts if one already exists

Why move types.generated?

Outputting generated types inside src/ has some drawbacks:

  • Linters and prettier will try to format the file. This has lead to issues in our CI already, as the type generator may not respect your indentation rules.
  • Putting generated types alongside your content can be confusing. We've already received support forum requests asking whether to ignore the file. By consolidating all generated types to .astro, we can have future-proof recommendations there!

Testing

  • astro check on updated @examples/with-content
  • check that writeFile is called on the env.d.ts file during astro sync

Docs

withastro/docs#2313

@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2023

🦋 Changeset detected

Latest commit: 4984511

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) pkg: example Related to an example package (scope) labels Jan 6, 2023
@bholmesdev bholmesdev force-pushed the feat/types-gen-astro-directory branch from 237db86 to 436c3b9 Compare January 6, 2023 22:19
@bholmesdev bholmesdev marked this pull request as ready for review January 6, 2023 23:12
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Jan 6, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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


```diff
/// <reference path="astro/client" />
+ /// <reference types="../.astro/types.d.ts" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean you don't get types without manually creating a file? Or was this already required? Is there anything we can do about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be automatically generated if a src/env.d.ts is present! Otherwise, we assume you've deleted or moved this file to manage ambient types yourself. Since this file is home to the astro/client types, I'd assume most users have this file in their Astro project.

@fflaten
Copy link
Contributor

fflaten commented Jan 7, 2023

Any reason this couldn't be in node_modules/.astro like the generated image cache? Just curious. 🙂

With astro sync available I'm unsure why we'd want to commit it anymore.

@bholmesdev bholmesdev force-pushed the feat/types-gen-astro-directory branch from 2ef697b to 44df368 Compare January 10, 2023 04:36
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

@bholmesdev
Copy link
Contributor Author

Fair question @fflaten! There's a couple reasons for it:

  1. To stay consistent with existing frameworks that use generated types, namely Nuxt and SvelteKit.
  2. To ensure editor tooling keeps up with your edits. Types in node_modules are cached by editors like VS Code, so you'd need to restart your TS server periodically while working in dev.

That second reason definitely pushed us to types in your project root. Luckily, .astro/ will be the landing place for any generated code in the future, making it easy to gitignore.

@fflaten
Copy link
Contributor

fflaten commented Jan 10, 2023

Thanks. Just not used to have generated data (cache) in a config-folder, but I agree the second reason alone is a good enough reason. 👍

@bholmesdev bholmesdev force-pushed the feat/types-gen-astro-directory branch from 3ce3b4f to 07ff9c8 Compare January 10, 2023 17:44
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Code LGTM, small comment about simplifying the .gitignore

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

@bholmesdev bholmesdev merged commit c218074 into main Jan 10, 2023
@bholmesdev bholmesdev deleted the feat/types-gen-astro-directory branch January 10, 2023 23:01
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) pkg: example Related to an example package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants