-
-
Notifications
You must be signed in to change notification settings - Fork 262
fix: properly migrate strict
and passthrough
#1262
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: properly migrate strict
and passthrough
#1262
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Pull Request Overview
This PR fixes the migration of Zod's .strict()
method to use Valibot's strictObject
function instead of the non-existent v.strict()
. The fix properly handles strict object schemas by using v.strictObject()
with the schema's entries when applicable.
- Replaces
v.strict()
calls withv.strictObject()
construction - Uses
.entries
property to extract object properties when converting chained calls - Updates test fixtures to reflect the corrected migration output
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
strict.ts |
Core transformation logic changed from adding v.strict() to pipe to creating v.strictObject() calls |
object-strip/output.ts |
Test fixture updated to show correct migration removing invalid v.strict() calls |
object-strict/output.ts |
Test fixture updated to show proper v.strictObject() usage with entries |
object-extend/output.ts |
Test fixture updated for extended objects with strict behavior |
return j.callExpression( | ||
j.memberExpression( | ||
j.identifier(valibotIdentifier), | ||
j.identifier('strictObject') | ||
), | ||
[j.memberExpression(schemaExp, j.identifier('entries'))] | ||
); |
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 transformation assumes schemaExp
always has an entries
property, but this may not be true for all schema types (e.g., variables referencing schemas). This could result in accessing .entries
on schemas that don't have this property, causing runtime errors.
Copilot uses AI. Check for mistakes.
commit: |
); | ||
const Schema18 = v.pipe(v.extend(Schema2, {bar: v.string()}), v.strict(), v.strip()); | ||
const Schema17 = v.object(v.extend(v.object({foo: v.string()}), {bar: v.number()}).entries); | ||
const Schema18 = v.object(v.extend(Schema2, {bar: v.string()}).entries); |
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.
Something looks wrong here. We don't have an v.extend()
function. We have v.entriesFromObjects()
but this is more a utility function. We could also just spread the .entries
. Spreading is probably cleaner for the codemod but I don't mind if it is simpler to implement with v.entriesFromObjects()
.
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.
v.entriesFromObjects()
exists for the rare case where you are only interested in the produced TS type but don't use the schema for validation. v.entriesFromObjects()
is a tree-shaking optimization. If you never use the schema at runtime bundlers like Rollup will remove it.
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.
This is another broken thing i plan to fix in a different pr...but the test needs to be updated because it's using strict
@@ -2,6 +2,8 @@ import * as v from "valibot"; | |||
|
|||
const Schema1 = v.strictObject({key: v.string()}); | |||
const Schema2 = v.strictObject({key: v.string()}, "some message"); | |||
const Schema3 = v.pipe(v.object({key: v.string()}), v.description("some description"), v.strict()); | |||
const Schema3 = v.strictObject( | |||
v.pipe(v.object({key: v.string()}), v.description("some description")).entries |
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.
This does not work. We are losing the pipe
this way. Better would be:
v.pipe(v.strictObject({key: v.string()}), v.description("some description"));
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.
This is related: #1262 (comment)
const Schema5 = v.strictObject(Schema4.entries); |
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.
This works but if people have the v.object
of Schema4
wrapped in a v.pipe
we will lose the pipe. The codemod will never be 100% perfect. So I am ok with merging this.
v.strict(), | ||
v.strip() | ||
const Schema10 = v.object( | ||
v.pipe(v.object({key: v.string()}), v.description("some description")).entries |
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.
This should probably be v.pipe(v.object({key: v.string()}), v.description("some description"))
but I know that it could be complicated to solve it. Basically, depending of the last Zod object modifier (strict
, strip
, ...) we need to choose the right Valibot schema (looseObject
, object
or strictObject
).
strict
strict
and passthrough
codemod/zod-to-valibot/__testfixtures__/object-extend/output.ts
Outdated
Show resolved
Hide resolved
codemod/zod-to-valibot/__testfixtures__/object-extend/output.ts
Outdated
Show resolved
Hide resolved
v.description("some description"), | ||
v.passthrough(), | ||
v.strip() |
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.
This looks wrong. It should use the last call of passthrough
, strip
or strict
and ignore and remove the previous calls to this modifier.
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.
So this should output v.pipe(v.object({key: v.string()}), v.description("some description"))
because strip
was the last modifier call.
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.
Uhhhh didn't notice this...let me check quickly
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.
Mmm i don't think this is completely correct tho right? I think .passthrough().strip()
will nullify passthrough
but .strict().strip()
shouldn't no? Because you first go over strict
and then try to strip (but strict
will already error before)...not super familiar with how zod
handles this.
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 am not totally sure but I think Zod just takes the last modifier and ignores all the other calls before that. I think strip
is the default behaviour and the others or similar to looseObject
and strictObject
in Valibot.
v.description("some description"), | ||
v.strict(), | ||
v.strip() |
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.
Same here
Other then that, I think the PR looks good. |
Currently
strict
migrate to the wrong code as you can see from the test input and output:input
output
v.strict()
is not a thing.this is the new output