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

Export AstroConfigType properly as type and not as a Zod schema value #7702

Merged
merged 3 commits into from Aug 15, 2023

Conversation

shishkin
Copy link
Contributor

@shishkin shishkin commented Jul 18, 2023

This makes the intended usage of Zod generated types actually possible

Changes

  • Export AstroConfigSchema as value instead of type
  • Export AstroConfigType properly as type and not as a Zod schema value

Testing

Previously, type autocomplete was broken on AstroConfig (e.g. inside an integration hook). With this change the zod schema value is retained and can be used to generate the proper type, so autocomplete works (e.g. in packages/astro/src/@types/astro.ts for AstroConfig type).

Docs

No docs impacted. This fixes previously intended behavior that wasn't working.

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2023

🦋 Changeset detected

Latest commit: 227254b

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: astro Related to the core `astro` package (scope) label Jul 18, 2023
@shishkin
Copy link
Contributor Author

@ematipico
Copy link
Member

ematipico commented Jul 18, 2023

@shishkin can you explain how you tested the feature? Maybe a screenshot to show this works now?

@shishkin shishkin requested a review from a team as a code owner July 18, 2023 17:33
@shishkin
Copy link
Contributor Author

shishkin commented Jul 18, 2023

@ematipico thanks for prompting me. I actually think my fix doesn't solve the problem I had.

The issue is that AstroConfig typing is not working in 3rd party integrations:

Screenshot 2023-07-18 at 18 40 04

I see only integrations defined in @types/astro.ts.

I thought this was due to the zod schema instance being exported as a type while it must be a value for the z.output<typeof ...> magic to work.

However, now even with my change I'm not able to get the autocompletion in an external integration project, though types now work internally within the astro monorepo for me.

It seems the zod type generation combined with interface inheritance doesn't survive in the exported type definitions. I finally had to resort to zod way of inheritance to generate the final AstroConfig type.

With latest changes I'm able to see autocompletions in an external integration project:

Screenshot 2023-07-18 at 19 34 14

@delucis
Copy link
Member

delucis commented Jul 19, 2023

I’m not sure this is currently not working. For example, in an integration I maintain, I already get autocomplete:
image

@shishkin
Copy link
Contributor Author

@delucis: I’m not sure this is currently not working. For example, in an integration I maintain, I already get autocomplete

I've tested your delucis/astro-embed repo and indeed autocomplete worked out of the box. But when I upgraded typescript to 5.1.6 and switched tsconfig module resolution to nodenext I only saw integrations in the autocomplete.

@delucis
Copy link
Member

delucis commented Jul 19, 2023

Good to know! I tested in https://github.com/withastro/starlight but still helpful to know this may be something introduced with specific config/dependency versions.

@shishkin
Copy link
Contributor Author

shishkin commented Aug 1, 2023

Is there an intention to merge this? Even if it's not an immediate concern for developers not using module resolution bundle or node16 there is still an error in how AstroConfigSchema is imported. If it's used as a value it can't be imported as a type.

@bluwy
Copy link
Member

bluwy commented Aug 2, 2023

@shishkin Could the issue be that our dts exported is not node16 compatible? There's work regarding it there at #7787. I don't think we'd want to export Zod schema as value, that's leaning towards internal API. And we probably don't want runtime in the astro.ts file too.

@shishkin
Copy link
Contributor Author

shishkin commented Aug 2, 2023

Yes, that makes sense. The issue is due to changes in module resolution bundler/node16. I can try to update the PR to ensure that the config package only exports a proper type instead of a zod schema value.

@shishkin shishkin changed the title Export AstroConfigSchema as value instead of type Export AstroConfigType properly as type and not as a Zod schema value Aug 13, 2023
@shishkin
Copy link
Contributor Author

@bluwy I've updated the PR to export the type and not the schema value. Hope this makes sense now and can be merged.

@ematipico ematipico merged commit c19987d into withastro:main Aug 15, 2023
14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Aug 15, 2023
@shishkin shishkin deleted the config-types branch August 15, 2023 07:46
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants