-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix(i18n): make i18n.routing
fields optional
#10165
Conversation
🦋 Changeset detectedLatest commit: d8154be 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 |
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was this tested? .default()
marks a field as optional already so this wouldn't change anything I don't think? I think the issue I noticed with types might come from the manual types file not the schema?
Turns out, it does not, unless that's a bug in zod. The current changes aren't woking. Doing The thing is, we need a default value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @types/astro.ts
changes look good! (Although there’s an indentation change I guess the auto-formatter will revert?)
Left a suggestion to remove the unnecessary .optional()
s in the schema.
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this so quickly @ematipico!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always forget how the types for the config interact too, oops.
Changes
Addresses the typing issue of #10157
The properties inside
i18n.routing
were incorrectly typed. Now they are optional.The type of
domains
was placed in the wrong part of the config... ooops! I fixed that tooTesting
It should compile and current tests should pass
Docs
N/A