-
Notifications
You must be signed in to change notification settings - Fork 5
Migrate zod to v4 #575
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
Migrate zod to v4 #575
Conversation
src/types.test.ts
Outdated
); | ||
}); | ||
|
||
it('should reject an invalid provider if default rpcUrl is missing', () => { |
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.
it('should reject an invalid provider if default rpcUrl is missing', () => { | |
it('should reject an invalid provider if either rpcUrl or homepageUrl is missing', () => { |
I think this test case isn’t specifically related to the default provider.
); | ||
}); | ||
|
||
it('should reject if "default" or "public" provider does not have rpcUrl', () => { |
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.
Can you also add provider with alias public
(with homepageUrl
attribute) into invalidProviders
array?
src/types.ts
Outdated
export const chainAlias = z.string().superRefine((value, ctx) => { | ||
if (!/^[\da-z-]+$/.test(value)) { | ||
ctx.addIssue({ | ||
code: 'invalid_format', | ||
format: 'regex', | ||
message: `Invalid chain alias format: ${value}`, | ||
}); | ||
return; | ||
} | ||
|
||
const isKnown = CHAINS.some((chain) => chain.alias === value); | ||
if (!isKnown) { | ||
ctx.addIssue({ | ||
code: 'custom', | ||
message: `Invalid chain alias: ${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.
export const chainAlias = z.string().superRefine((value, ctx) => { | |
if (!/^[\da-z-]+$/.test(value)) { | |
ctx.addIssue({ | |
code: 'invalid_format', | |
format: 'regex', | |
message: `Invalid chain alias format: ${value}`, | |
}); | |
return; | |
} | |
const isKnown = CHAINS.some((chain) => chain.alias === value); | |
if (!isKnown) { | |
ctx.addIssue({ | |
code: 'custom', | |
message: `Invalid chain alias: ${value}`, | |
}); | |
} | |
}); | |
export const chainAlias = aliasSchema.refine((value) => CHAINS.some((chain) => chain.alias === value), { | |
error: (iss) => `Invalid chain alias: ${iss.input}`, | |
}); |
I'd stick to original implementation in the v4 way.
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 have tried that at first but tests were failing due to a bug in zod:
expect(received).toThrow(expected)
- Expected message - 0
+ Received message + 1
[
{
+ "origin": "string",
"code": "invalid_format",
"format": "regex",
"pattern": "/^[\\da-z-]+$/",
"path": [],
"message": "Invalid string: must match pattern /^[\\da-z-]+$/"
},
{
"code": "custom",
"path": [],
"message": "Invalid chain alias: Mantle"
}
]
I have added origin
but then I got this error:
Object literal may only specify known properties, and 'origin' does not exist in type '$ZodIssueInvalidStringFormat'.ts(2353)
Which is true and can be seen here. But origin
is being added anyway like following ZodCheckStringFormat
There are some issues related to regex on zod repo but I couldn't find any about this specific situation. But you can see the actual behavior is too emit origin
here is an issue.
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.
Yes, you're right. I got same results with yours.
I gave some tries and changing tests in the following way solved the problem you mentioned, wdyt?:
//1
it('should reject chain aliases not in the CHAINS array', () => {
const resultInvalidFormat = chainAlias.safeParse('Mantle');
expect(resultInvalidFormat.error).toBeInstanceOf(ZodError);
expect(resultInvalidFormat.error?.issues).toStrictEqual([
{
origin: 'string',
code: 'invalid_format',
format: 'regex',
pattern: '/^[\\da-z-]+$/',
path: [],
message: 'Invalid string: must match pattern /^[\\da-z-]+$/',
},
{ code: 'custom', path: [], message: 'Invalid chain alias: Mantle' },
]);
const resultInvalidChainAlias = chainAlias.safeParse('ethereum-mainnet');
expect(resultInvalidChainAlias.error).toBeInstanceOf(ZodError);
expect(resultInvalidChainAlias.error?.issues).toStrictEqual([
{ code: 'custom', path: [], message: 'Invalid chain alias: ethereum-mainnet' },
]);
});
//2
it('accepts valid chain IDs and throws on invalid ones', () => {
const validChainsDapp = {
aliases: {
'valid-chains': {
chains: ['ethereum', 'bsc', 'polygon'],
title: 'Valid Chains DApp',
},
},
homepageUrl: 'https://example.com',
};
expect(() => dappSchema.parse(validChainsDapp)).not.toThrow();
const invalidChainsDapp = {
aliases: {
'invalid-chains': {
chains: ['ethereum', 'invalid-chain', 'polygon'],
title: 'Invalid Chains DApp',
},
},
homepageUrl: 'https://example.com',
};
const resultInvalidChainsDapp = dappSchema.safeParse(invalidChainsDapp);
expect(resultInvalidChainsDapp.error).toBeInstanceOf(ZodError);
expect(resultInvalidChainsDapp.error?.issues).toStrictEqual([
{
code: 'custom',
path: ['aliases', 'invalid-chains', 'chains', 1],
message: 'Invalid chain alias: invalid-chain',
},
]);
});
path: ['providers', 'alias'], | ||
message: "a provider with alias 'default' is required", | ||
input: providers.map((p) => p.alias), |
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.
Using the whole providers
object is another option, but I'd also prefer using only the mapped relevant values 👍🏼.
That being said, since this issue is at the array level and we’re passing the whole array as input
, I’d leave the path
attribute empty []
. Basically, my main approach is: if it’s an array-level error, leave the path empty; if it’s an error for a specific element in the array, set the path
precisely to that element.
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.
It is an array error indeed, leaving it empty might be preferable
src/types.ts
Outdated
path: ['providers', 'rpcUrl'], | ||
message: "providers with alias 'default' or 'public' must also have an 'rpcUrl'", | ||
input: p.rpcUrl, |
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.
path: ['providers', 'rpcUrl'], | |
message: "providers with alias 'default' or 'public' must also have an 'rpcUrl'", | |
input: p.rpcUrl, | |
path: [index, 'rpcUrl'], | |
message: "providers with alias 'default' or 'public' must also have an 'rpcUrl'", | |
input: p.rpcUrl, |
If you'd like to follow my approach mentioned above.
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 will replace providers
with index. Easy 😂😂😂
@@ -0,0 +1,5 @@ | |||
--- | |||
'@api3/contracts': major |
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.
👍🏼
Closes #574
This PR upgrades the zod package to version 4.
In addition to the migration, missing test cases have been added to ensure full coverage and validation of the updated schema logic.