More Structured format for parameters and Enhanced FES #7843
Conversation
|
Hi @davepagurek as part of the extensive work done for #7752 , here I am opening a PR to generate more informative and structural json format for The most trickiest part I encountered to handle was - @ksen0 , @limzykenneth as it is a heavy change I would like to request your feedback as well. Just like to inform that this PR has been made by detailed discussions through exchange of thoughts between me and @davepagurek and consensus on the structure of the json to be build upon in the issue #7752 . I hope you all would like my work. And yes, Hoorrray! all the tests are also got passed, I felt that some of the flaky tests might get appeared again as sometimes it might appears. |
There was a problem hiding this comment.
Thanks for your work on this, it's looking good!
So it looks like currently we're using typeObject and then have to re-parse structure out of the strings it returns. I'm wondering if maybe we can make a new function, e.g. structuredTypeObject, that does something similar to the original but preserves more of the original structure instead of condensing it into a string, while keeping around the usual typeObject function so that the output data.json (which is used on the p5 website) doesn't change?
| return { | ||
| type: 'Function', | ||
| parameters, | ||
| ...(returnType && { returnType }) // conditionally include returnType |
There was a problem hiding this comment.
Cool, I didn't know ... worked on undefined without errors, this is good to know!
There was a problem hiding this comment.
Thanks! Yeah, it's a handy little pattern — the spread operator with a short-circuit like ...(returnType && { returnType }) is a nice way to conditionally include properties without mutating the object. I actually picked this up while working with TypeScript, where patterns like this come up often in handling optional fields in typed object literals. Even though this is a JS file, bringing in some of those TS habits has definitely helped me keep things cleaner and more predictable.
| // If the param is a function, populate parameters explicitly (only if not already done) | ||
| if ( | ||
| typeInfo.type === 'Function' && | ||
| !current[key].parameters && |
There was a problem hiding this comment.
Do you know what circumstances would lead to having a function here but where typeObject(tag.type) doesn't return an object with a parameters key? I wonder if, rather than special-casing functions here, we can address that case in typeObject instead.
There was a problem hiding this comment.
Good catch — you're right to raise this. I have already handled this in typeobject with node type as FunctionType:
else if (node.type === 'FunctionType') {
const parameters = node.params.map(p => typeObject(p));
const returnType = node.result ? typeObject(node.result).type : undefined;
return {
type: 'Function',
parameters,
...(returnType && { returnType })
};
}This check here in convert.mjs was meant as a safeguard in case some tag didn’t pass through typeObject properly or got overridden with incomplete type info — for example, if a parent param was documented vaguely or multiple @param(s) with overlapping paths were present. That said, if we trust that typeObject always runs and always returns the correct structure, then I agree this special-case logic may be redundant and we could enforce that structure more cleanly within typeObject itself.
Happy to refactor accordingly if we want to centralize the responsibility in typeObject.
|
|
||
| } else { | ||
| // Intermediate node: create object structure for nested params | ||
| current[key] = current[key] || { type: 'Object', optional: true, properties: {} }; |
There was a problem hiding this comment.
There's possibly a case here where we don't want this to be optional. Maybe if we say every time you have an object with properties, you have to document the object itself first? e.g. if you have:
/**
* @param {Object} options
* @param {number} options.sampleFactor
*/...then as long as we add the less-deeply-nested @param first, we can use whatever type info we get from it. We could then possibly throw an error here if there's a deeply nested property documented before its parent (since we likely don't want that in the docs anyway.)
There was a problem hiding this comment.
Yes, Dave, I agree with your point. Initially, I added the optional: true as a safeguard in case the parent object wasn't explicitly documented. But on second thought, this scenario shouldn't occur at all — we should require that the parent object be documented before any of its nested properties. So, I agree that throwing an error here is the right approach
| // Convert numeric-keyed object or array to array of processed params | ||
| if (Array.isArray(param.parameters)) { | ||
| result.parameters = param.parameters.map(processParam); | ||
| } else if (typeof param.parameters === 'object') { |
There was a problem hiding this comment.
Do you know under what circumstances we end up with an object here?
There was a problem hiding this comment.
yes, it somehow generates as follows, possibly from the overloads of loadModel, createModel . So to handle those I used the above as safeguards
{ '0': { type: 'Event' } }
{ '0': { type: 'p5.Geometry' } }| } | ||
|
|
||
| // Parse union type strings | ||
| const parseUnion = unionStr => { |
There was a problem hiding this comment.
Rather than parsing out arrays, tuples, etc here, could we modify typeObject to return something structured? Since we start with the structure, rather than condensing it and re-parsing it, we could just keep the original structure.
There was a problem hiding this comment.
Of Course! why not we could definitely modify the typeObject without changing the output of data.json it's just that we need to handle all the corner and edge cases as how I handled explicitly for arrays, tuples, unions, etc.
The original structure just handles the top level parsing and not the deep nested ones so require handling of these explicitly to generate more structured output
|
Hi @davepagurek, thank you so, so , so... much for reviewing!
Regarding your concern about changes to the data.json output used on the p5 website — I’ve tested the PR locally, and from what I observed, the output format hasn't changed. The only addition is a block related to /**
* Recursively builds a Zod schema from a type descriptor object.
*
* @param {object} typeObj - An object describing the type.
* @returns {ZodType} - A Zod schema representing the type.
*/I believe this is unrelated to the core logic in That said, I'm happy to revise or remove anything you think could lead to unintended effects — just let me know what you'd prefer! |
|
Hi @davepagurek, thanks again for your thoughtful feedback throughout this PR. From what I understand based on your comments above, you're suggesting that rather than handling special cases like functions externally (e.g. in Just checking to confirm: is the goal here to simplify the usage of If that's the direction you'd prefer, I’m happy to refactor accordingly. Please let me know if I’ve understood your vision correctly — I’ve taken a moment to carefully think through your points and want to ensure we’re aligned before moving forward with revisions. |
I think so, since it would simplify the process of structured --> string --> parsed string --> structured to just structured --> different structured. The one caveat is that this is just for producing |
Yes, I agree — it's important to keep the
Regarding the idea of having two separate functions: I was thinking we could potentially keep things simpler by using a single function that conditionally outputs either the structured type (for |
|
Sounds good, thanks @IIITM-Jay! |
Resolves #7752
Overall Description
This PR deals with generating more structured and informative
parameterData.jsonin order to get a clear and accurate idea of the various parameters involved in overloads of methods. Additionally, as a part of the enhancement process, this PR also resolves and enhances the ability to check thetypesof the nested as well as positional parameters efficiently at run time, which is also one of the critical aspect of the issue.Approach Used
Enhancements made to be able to build more structural format to
parameterData.jsonFunctionTypenode are extracted and preserved for further processingModified FES to parse and check the types at run time via generated
Zod schema?) asendsWith('?')is no longer requiredSwitch- Caseis used to handle mappings fromtypenames toZod schemasAdditional Code Quality Work
Trickiest Portions to handle
[p5.Color|String|Number|Number[], Number][]"...Number[]"function(p5.Geometry)Observations and Results
parameterData.jsonto verify the expected resultsAdditional Checks
npm run docs- to generate theparameterData.jsondocnpm run lint- lint checker for code style and standardsnpm test- to re-validate all the test suites passed