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

chore(deps): Upgrade Zod to @latest #8762

Merged
merged 13 commits into from
Oct 30, 2023
Merged

Conversation

evadecker
Copy link
Contributor

@evadecker evadecker commented Oct 6, 2023

Changes

Testing

  • No tests added

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2023

🦋 Changeset detected

Latest commit: e668df3

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: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Oct 6, 2023
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.

Thank you for the contribution!

@natemoo-re
Copy link
Member

Not sure what's going on with the CI failure, but it's definitely not your fault! We'll debug that.

@bluwy
Copy link
Member

bluwy commented Oct 10, 2023

Zod is specifically pinned because of that types issue. I spent quite some time on it before and it seems there's no way to workaround it (tsc just bails). The only way if we really want it to work is to unwrap this as a standalone type i think:

export type AstroConfigType = z.infer<typeof AstroConfigSchema>;

@evadecker
Copy link
Contributor Author

@bluwy Thanks for that, I tried digging a little bit, too, but this level of TypeScript is a bit beyond me. Do you know why the pinned version of zod works but later releases don't?

@bumu
Copy link

bumu commented Oct 27, 2023

@evadecker can you help to fix the conflicts?

@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Oct 27, 2023
@evadecker
Copy link
Contributor Author

@evadecker can you help to fix the conflicts?

I reran pnpm i to get rid of the lockfile merge conflict, but we still have the issue that @bluwy mentioned.

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 updating the verb tense of the changeset for consistency! Also, when just using the name of the thing, Zod appears to be capitalized, so did that too.

Docs will be happy with these changes! 🙌

.changeset/swift-rivers-impress.md Outdated Show resolved Hide resolved
@evadecker evadecker changed the title chore(deps): Upgrade zod to @latest chore(deps): Upgrades Zod to @latest Oct 28, 2023
@evadecker evadecker changed the title chore(deps): Upgrades Zod to @latest chore(deps): Upgrade Zod to @latest Oct 28, 2023
@evadecker
Copy link
Contributor Author

evadecker commented Oct 28, 2023

@bluwy @natemoo-re

I was able to resolve the previous error—

Error: src/core/config/schema.ts(62,14): error TS2742: The inferred type of 'AstroConfigSchema' cannot be named without a reference to '.pnpm/[email protected]/node_modules/mdast-util-to-hast'. This is likely not portable. A type annotation is necessary.
Error: src/core/config/schema.ts(312,17): error TS2742: The inferred type of 'createRelativeSchema' cannot be named without a reference to '.pnpm/[email protected]/node_modules/mdast-util-to-hast'. This is likely not portable. A type annotation is necessary.

—without unwrapping AstroConfigType as a standalone type. 🎉

To do this required explicitly adding mdast-util-to-hast as a devDependency (pinned @12.3.0), and importing mdast-util-to-hast in the config file.

The extra import feels odd to me, but it's the solution suggested by the TypeScript team developer lead when TypeScript's declaration emitter fails to resolve a reference.

@natemoo-re
Copy link
Member

@evadecker wow great job! I think that works. I'm curious if we'd need to add it as an actual dependency to ensure it's included in user's projects, though.

@evadecker
Copy link
Contributor Author

evadecker commented Oct 28, 2023

Happy to make an edit if that's the case!

Not sure why macos-latest test failed: hopefully just a flaky test?

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

@bluwy could you please take a look?

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

One small request, but great work on fixing this! Looks like the move from shiki to shikiji allowed an easier fix here.

packages/astro/package.json Outdated Show resolved Hide resolved
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.

Thanks so much for the PR @evadecker!

@natemoo-re natemoo-re merged commit 35cd810 into withastro:main Oct 30, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 30, 2023
@evadecker evadecker deleted the upgrade-zod branch October 30, 2023 21:04
natemoo-re added a commit that referenced this pull request Nov 22, 2023
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Nate Moore <[email protected]>
Co-authored-by: Nate Moore <[email protected]>
@bluwy bluwy mentioned this pull request Feb 26, 2024
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: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[email protected] and @astrojs/[email protected] using vulnerable version of Zod
6 participants