Conversation
There was a problem hiding this comment.
In addition to the zod-related changes, we'll also need to bump the version number of the CLI itself to set up the new release of the CLI. Since it's just an internal change, we can do a patch update to v5.4.1. Although keep an eye out, as there is another open PR that bumps to v5.5.0. If that goes in first, this will have to take us up to v5.5.1.
In case you need it, here's the full instructions for making CLI updates, including all the steps that will come after this PR: https://www.notion.so/dittov3/Working-on-Ditto-Dev-Tools-ad7b4a133c4b4327961c0af4ba2c7637
marla-hoggard
left a comment
marla-hoggard
left a comment
There was a problem hiding this comment.
As far as I can tell, without deep knowledge of all the different export formats, everything is looking good/consistent with master. See my comments in the code - there's a few more things we can/should update. And then we'll need to bump the package version before this can merge.
lib/src/services/projectConfig.ts
Outdated
There was a problem hiding this comment.
I noticed that flatten() was showing up as deprecated now as well. Looks like we can change it to this, which looks a little nicer too:
| formattedError: z.prettifyError(parsedYAML.error), |
BEFORE
AFTER
lib/src/outputs/android.ts
Outdated
| export const ZAndroidOutput = z.strictObject( | ||
| ZBaseOutputFilters.extend({ | ||
| format: z.literal("android"), | ||
| framework: z.undefined().optional(), |
There was a problem hiding this comment.
Since these are strict objects, I believe we can actually delete the framework field from every one of these definitions except for the few that have a literal value defined.
My assumption was that these were here because ZBaseOutputFilters had framework defined and this was overriding that field back to undefined or optional as the only valid values. However, this is not the case. framework is not defined on this base type so there shouldn't be any reason to explicitly add it as accepting undefined.
In testing, I've found that adding framework: value or framework: undefined or framework: to these outputs (I happened to test the android format) has all the same behavior on this branch, on master, and on this branch with that field removed.
So, rather than add optional() to each of these z.undefined() frameworks, let's remove the field entirely.
|
Oh, I just saw you're out this week. I can take care of the requested changes and then will ask another dev for final review. |
|
|
||
| public async format(): Promise<void> { | ||
| const data = await this.fetchAPIData(); | ||
| const files = await this.transformAPIData(data); |
There was a problem hiding this comment.
Not zod related, but this is a sync method.
|
|
||
| export const ZAndroidOutput = ZBaseOutputFilters.extend({ | ||
| format: z.literal("android"), | ||
| framework: z.undefined(), |
There was a problem hiding this comment.
I moved the framework: undefined definition to ZBaseOutputFilters so it only needs to be declared on a subtype if it's not undefined, rather than manually including undefined on every subtype.
|
@bparrish17 I took this over from Laura since she is on PTO now. Do you mind doing a final review after my latest commits? |
- Replace .merge() with .extend() (deprecated in v4) - Replace .strict() with z.strictObject() (deprecated in v4) - Add explicit key schema to z.record() (single-arg no longer supported) - Make z.undefined() fields optional to preserve v3 behavior
marla-hoggard
force-pushed
the
laura/update-zod3-to-zod4-cli
branch
from
40cf2e2 to
88a6bcf
Compare
bparrish17
left a comment
bparrish17
left a comment
There was a problem hiding this comment.
tested a bunch of outputs and everything looks good to me 👍 thanks for getting this across the finish line for laura
3 participants
Overview
Update zod 3 to zod 4
Context
Screenshots
Test Plan
Testing successfully completed in via: