p5.js icon indicating copy to clipboard operation
p5.js copied to clipboard

More Structured format for parameters and Enhanced FES

Open IIITM-Jay opened this issue 6 months ago • 6 comments

Resolves #7752

Overall Description

This PR deals with generating more structured and informative parameterData.json in 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 the types of 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.json

    • The parameters of FunctionType node are extracted and preserved for further processing
    • Traversed more deeper to build more informative structure for supporting nested params
    • Used appropriate regular expression as required at appropriate places
    • Efficient handling of:
      • Array Types
      • Union Types
      • Tuple Types
      • Function Parameters
      • Nested Object Parameters
      • Rest Parameters
      • Array of Tuples consisting of nested union and other params
  • Modified FES to parse and check the types at run time via generated Zod schema

    • Now, string checking via the presence of (?) as endsWith('?') is no longer required
    • Switch- Case is used to handle mappings from type names to Zod schemas

Additional Code Quality Work

  • Used fallback mechanism for safety and to avoid runtime errors
  • Explicit handling of types to ease overall maintainability and readability
  • Added in-line comments as appropriate

Trickiest Portions to handle

  • Nested Parameters like [p5.Color|String|Number|Number[], Number][]
  • Rest Parametrs "...Number[]"
  • Functions with parameters function(p5.Geometry)

Observations and Results

  • Tested and validated the parameterData.json to verify the expected results

Additional Checks

  • [x] npm run docs - to generate the parameterData.json doc
  • [x] npm run lint - lint checker for code style and standards
  • [x] npm test - to re-validate all the test suites passed

IIITM-Jay avatar May 24 '25 19:05 IIITM-Jay

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 parameterData.json. I really enjoyed doing it. Also, attached paarmeterData.json to verify the results as expected. More detailing I have given in the PR and also the file contains in-line comments as appropriately required. Requesting you to have a look and review on this PR.

The most trickiest part I encountered to handle was - [p5.Color|String|Number|Number[], Number][] where I need to handle Array of Tuples where Tuple contains unions where union again consists of varied types as arrays, strings etc. etc.

@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.

IIITM-Jay avatar May 24 '25 19:05 IIITM-Jay

Hi @davepagurek, thank you so, so , so... much for reviewing!

the output data.json (which is used on the p5 website) doesn't change?

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 generateZodSchemasForFunc, which appears because of the new JSDoc comment added for buildSchema() in param_validator.js.

/**
   * 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 convert.mjs, and doesn't affect existing functionality — just adds new schema generation support. It's possible the changes in param_validator.js might have caused some confusion here, so just flagging that in case it was missed during review 🙂

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!

IIITM-Jay avatar May 31 '25 13:05 IIITM-Jay

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 convert.mjs), we should instead centralize all type structure logic within typeObject() itself — ensuring it always returns a fully structured and consistent output (e.g., including parameters for functions when relevant).

Just checking to confirm: is the goal here to simplify the usage of typeObject() by making it the single source of truth for all type-related processing, so downstream methods using it don’t need to do any further conditional handling?

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.

IIITM-Jay avatar May 31 '25 13:05 IIITM-Jay

Just checking to confirm: is the goal here to simplify the usage of typeObject() by making it the single source of truth for all type-related processing, so downstream methods using it don’t need to do any further conditional handling?

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 parameterData.json for now -- I think it's OK to keep the format the same as it currently is for data.json and data.min.json, which is also generated from convert.mjs and is what gets displayed on the website. For FES, we need the deep structure because we need to check all the properties of objects, whereas on the website, we need a single string for each parameter to display. So I think that maybe means having two functions to produce types, one used when generating each file? One generating structured data, one just generating type strings as before?

davepagurek avatar Jun 01 '25 12:06 davepagurek

I think it's OK to keep the format the same as it currently is for data.json and data.min.json, which is also generated from convert.mjs and is what gets displayed on the website.

Yes, I agree — it's important to keep the data.json and data.min.json formats unchanged, especially since they're used for the website and are derived from convert.mjs. That’s exactly why I’ve been careful to make changes in a way that doesn't affect those files.

So I think that maybe means having two functions ...

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 FES) or the stringified type (for website display), depending on the use case. I’ll explore both approaches — separate vs conditional within one — and choose the one that offers the best balance of clarity, maintainability, and long-term flexibility, as to keep as simpler as possible for the longer run sustainability.

IIITM-Jay avatar Jun 01 '25 16:06 IIITM-Jay

Sounds good, thanks @IIITM-Jay!

davepagurek avatar Jun 02 '25 12:06 davepagurek

Just want to make sure that we are keeping things compatible and not duplicating things between this PR and #7863 (I don't think it is but just to mention), since they are touching related things in parallel.

One thing I think would be very nice to have for this part of FES, or more specifically the parameter checks part of FES, is to have some unit tests that includes the edge cases identified, while we continue to refine this part so that we don't have to worry about regression. If it is simple to do at least a skeleton of it, we can do it in this PR or if it looks like a larger endavour we can do it separately and get this in first.

limzykenneth avatar Jun 20 '25 13:06 limzykenneth

Also, just wanted to check in again @IIITM-Jay, I think the last things to do here were to update typeObject to remove the need to re-parse any strings, and to confirm again afterwards that data.json is unmodified. Let me know if there's anything else I can help with to get this across the finish line and merged 🙂

davepagurek avatar Jun 20 '25 21:06 davepagurek

Hi @IIITM-Jay, thanks so much for all your work on this. Could you confirm please if you are still working on this PR? Please check in in the next 7 days (no need to be fully finished in the next 7 days, but to check in whether you are working on it still), otherwise we will finish up this PR and get it merged in. Thank you!

ksen0 avatar Sep 16 '25 12:09 ksen0