-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Update param validator and test file #7268
Conversation
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.
Nice work, this is looking good!
function validateParams(p5, fn) { | ||
// Cache for Zod schemas | ||
let schemaRegistry = new Map(); | ||
const arrDoc = JSON.parse(JSON.stringify(dataDoc)); |
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.
Is this duplication because we modify the 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.
What do you mean by duplication here?
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 JSON.parse(JSON.stringify(...)) bit, normally I see that used to make a deep copy of an object
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.
If there is a need for duplicating object use structuredClone()
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.
We also might not need to do a clone at all here if we're only reading the data and not modifying it later
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.
Ahh yes! I actually got this line of code from the old parameter validation and didn't think too much about it. Changed the code so that it uses dataDoc
directly.
overloads = funcInfo.overloads; | ||
// `constantsMap` maps constants to their values, e.g. | ||
// { | ||
// ADD: 'lighter', |
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.
These examples in the comments are really helpful, nice!
'Number[]': z.array(z.number()), | ||
'Object': z.object({}), | ||
// Allows string for any regex | ||
'RegExp': z.string(), |
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.
Do we want to also allow RegExp instances here too?
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.
Not sure what the usage of RegExp instances was like previously, happy to add it if it's good to include!
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.
What I can find is RegExp is used in p5.Table.matchRow()
, there may be more places but not many. To check for regex properly it probably should be checking whether the argument is an instance of RegExp
.
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.
Replaced string for regexp with the web API object!
// Generate a schema for a single parameter. In the case where a parameter can | ||
// be of multiple types, `generateTypeSchema` is called for each type. | ||
const generateParamSchema = param => { | ||
const optional = param.endsWith('?'); |
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 might be a good idea to handle arrays like this too when generating a param schema by checking for something that ends with []
to save having to manually handle array versions of each type
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.
Sounds good!
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.
Just pushed another commit that handles []
logic dynamically!
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 changes look great, thanks!
Addresses #7178
Changes:
p5
, testing web API objects)PR Checklist
npm run lint
passes