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(next): reference astro/client from astro/config #11925

Merged
merged 34 commits into from
Sep 11, 2024

Conversation

florian-lefebvre
Copy link
Member

@florian-lefebvre florian-lefebvre commented Sep 5, 2024

Changes

  • Reference astro/client from astro/config
  • Swaps the order of our default tsconfig include to avoid overriding types

Testing

Manually

Docs

Changeset. I'll submit a docs PR if this one is merged

Copy link

changeset-bot bot commented Sep 5, 2024

🦋 Changeset detected

Latest commit: 006deef

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 the pkg: example Related to an example package (scope) label Sep 5, 2024
@bluwy
Copy link
Member

bluwy commented Sep 5, 2024

Is this related to the @ts-check PR? it feels odd to add this in the config file.

@florian-lefebvre
Copy link
Member Author

It's unrelated. It's the only file I found is in all our starters than can hold this reference (now that env.d.ts is optional)

@bluwy
Copy link
Member

bluwy commented Sep 5, 2024

In that case, can you explain why we need this? Wouldn't there be other types errors if astro sync has not been run yet?

@florian-lefebvre
Copy link
Member Author

The idea is to get the basic types (thing import.meta.env, astro:action defineAction etc) out of the box, without relying on codegen. That means that users creating a project will not wonder why most things are broken before running dev for example. It won't solve everything (eg. precise content types) but better than nothing. I made this PR after feedback from @delucis

@florian-lefebvre
Copy link
Member Author

@Princesseuh suggests that we add this reference in astro/config instead. I think it's a great idea, wdyt? I don't think there's a case where you'd want to import astro/config without getting those basic types

@bluwy
Copy link
Member

bluwy commented Sep 7, 2024

Fine by me if it works. I'm not sure if TypeScript works correctly if referenced this way.

@florian-lefebvre
Copy link
Member Author

I know @Fryuni used to rely on that for inox tools, and i think there was a few gotchas. Do you mind reminding us what they are?

@Fryuni
Copy link
Member

Fryuni commented Sep 7, 2024

Adding references, directly or indirectly, on the Astro config only works if the config file is being analyzed by TS.

All on Inox Tools provide the virtual modules indirectly, just like the idea of having the reference in astro/config. It works fine for the templates, but users that use their own TSconfig probably won't have the types.

It requires on the following configs to work:

  • allowJs must be true (not the default)
  • isolatedModules must be false (the default)
  • The config file must be included in the project (match an include rule and no exclude, the default works)

The TSconfig files shipped with Astro all works, but an empty one using TS defaults won't. A custom one maybe works.

@florian-lefebvre
Copy link
Member Author

allowJs: true is enabled by default in our presets so I think it's fine

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 9, 2024
@florian-lefebvre florian-lefebvre changed the title feat(next): add astro/client references to examples feat(next): reference astro/client from astro/config Sep 9, 2024
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Sep 9, 2024
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.

@florian-lefebvre
Copy link
Member Author

Interestingly, I had to swap the order of our default tsconfig include array, otherwise the new reference in astro/config would override the one from .astro/types.d.ts

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 9, 2024
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

@florian-lefebvre florian-lefebvre merged commit 74722cb into next Sep 11, 2024
14 checks passed
@florian-lefebvre florian-lefebvre deleted the feat/next-config-client-reference branch September 11, 2024 12:40
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) pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants