-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
error fixed #8109
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
base: dev-2.0
Are you sure you want to change the base?
error fixed #8109
Conversation
'Function': z.function(), | ||
'Integer': z.number().int(), | ||
'Number': z.number(), | ||
'Number': z.union([z.number(), z.literal(Infinity), z.literal(-Infinity)]), |
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.
was there's a reason to use this instead of z.number().or(z.literal(Infinity))
as per dave's suggestion?
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.
z.number().or(z.literal(Infinity)) only allows finite numbers and Infinity, but not -Infinity.
z.union([z.number(), z.literal(Infinity), z.literal(-Infinity)]) allows finite numbers, Infinity, AND -Infinity.
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.
Maybe this should be better alternative : z.number().or(z.literal(Infinity)).or(z.literal(-Infinity));
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.
Hi @Nitin2332, sorry for the delay in reviewing this. I didn't got time to debug this.
After looking at the errors, which says:
FAIL |unit| test/unit/core/param_errors.js > Validate Params > validateParams: multi-format > color(): optional parameter, incorrect type
FAIL |unit| test/unit/core/param_errors.js > Validate Params > validateParams: multi-format > color(): extra parameter
FAIL |unit| test/unit/core/param_errors.js > Validate Params > validateParams: multi-format > color(): incorrect element type
AssertionError: expected '🌸 p5.js says: Expected number or con…' to equal '🌸 p5.js says: Expected number at the…'
Expected: "🌸 p5.js says: Expected number at the fourth parameter, but received string in p5.color()."
Received: "🌸 p5.js says: Expected number or constant (please refer to documentation for allowed values) at the fourth parameter, but received a in p5.color()."
Looks like your changes broke the friendly error string builder by turning every Number into a union (number | Infinity | -Infinity).
How?
Like before when it was z.number()
the friendly error must be something like "Expected number but recived.......", now when you changed z.number
to z.union([z.number(), z.literal(Infinity), z.literal(-Infinity)]),
now we have two other literals as +-Infinity
?
Here's only the catch, Infinity is treated as a number in javascript, but our zod schema mapped this as a literal z.literal(+-Infinity)
, that's why the message builder is looking it as a constant.
So, now the issue is that where we should get a message as
expectedTypes.add('constant (please refer to documentation for allowed values)'); |
infinity
and -infinity
, we are getting this error :
Expected: "🌸 p5.js says: Expected number at the fourth parameter, but received string in p5.color()."
Received: "🌸 p5.js says: Expected number or constant (please refer to documentation for allowed values) at the fourth parameter, but received a in p5.color()."
What should you can do to fix this?
So, tests wants just number instead of number or constant both to pass.
For that, you need to flatten the sub-issues of union and if you see numbers are present along with the literals (literals only with +-Infinity), delete the constant part.
Maybe it should be something like,
if( number && litralwithplusminusinfinity) {
expectedTypes.delete('constant (please refer to documentation for allowed values)');
}
Feel free to discuss or ask help if you face issues in fixing this. Thanks for your help.
Also, tagging @davepagurek if you got any other fix for this? |
ahh good catch, we will have to add a special case to handle the number-or-infinity union. that makes sense @perminder-17! |
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.
Hi @Nitin2332 , thanks for your work on this, but I think the code has some issues which I mentioned below. Can you please look and stick to the approach I mentioned above? Also, I am unclear with what approach you are looking at.
} | ||
// Handle nested union errors (e.g., Number type with number, Infinity, -Infinity) | ||
else if (issue.code === 'invalid_union') { | ||
issue.errors.forEach(nestedErr => { |
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 nested failure invalid_union
is inside issue.unionErrors
. See this doc: https://raftar.io/chene/interfaces/chene.z.ZodInvalidUnionIssue.html
// Handle nested union errors (e.g., Number type with number, Infinity, -Infinity) | ||
else if (issue.code === 'invalid_union') { | ||
issue.errors.forEach(nestedErr => { | ||
const nestedIssue = nestedErr[0]; |
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.
nestedErr
won't be an array anymore, it's a zod object?
// constant values in the error message, the error message will | ||
// direct users to the documentation. | ||
// Note: Distinguish actual constants from special number values like Infinity/-Infinity | ||
else if (issue.code === 'invalid_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.
Like you are checking inside the condition for literals infinity
and -Infinity
but when the union gets failed we check it from : issue.code === 'invalid_union'
else if (issue.code === 'invalid_value') { | ||
expectedTypes.add('constant (please refer to documentation for allowed values)'); | ||
actualType = args[error.path[0]]; | ||
const isSpecialNumber = issue.values && issue.values.some(val => val === Infinity || val === -Infinity); |
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.
You will never see literals, so you will always get isSpecialNumber
to false.
actualType = args[error.path[0]]; | ||
const isSpecialNumber = issue.values && issue.values.some(val => val === Infinity || val === -Infinity); | ||
if (!isSpecialNumber) { | ||
hasConstants = true; |
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.
since isSpecialNumber
is always false, hasConstants
will be always set to true.
|
||
// Only add constants to expected types if constant validation actually failed | ||
if (hasConstants) { | ||
expectedTypes.add('constant (please refer to documentation for allowed values)'); |
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 think the tests are accidentally passing, since hasConstants
always comes out to true.
Resolves #8104
PR Checklist
npm run lint
passes