[2.0]: Properties of objects aren't being checked correctly by FES

[davepagurek]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

2.0.0

Web browser and version

Firefox

Operating system

MacOS

Steps to reproduce this

Steps:

If you run npm run docs and then check docs/parameterData.json, if you search for textToModel, it currently has these parameters:

"textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "Object?",
          "Number?",
          "Number?"
        ]
      ]
    }

This is because we're documenting the properties of the object like this:

* @param {String} str The text string to convert into a 3D model.
* @param {Number} x The x-coordinate for the starting position of the text.
* @param {Number} y The y-coordinate for the starting position of the text.
* @param {Number} width Maximum width of the text block (wraps text if exceeded).
* @param {Number} height Maximum height of the text block.
* @param {Object} [options] Configuration options for the 3D text:
* @param {Number} [options.extrude=0] The depth to extrude the text. A value of 0 produces
* flat text; higher values create thicker, 3D models.
* @param {Number} [options.sampleFactor=1] A factor controlling the level of detail for the text contours.
*  Higher values result in smoother curves.

It looks like we're mistakenly parsing options.extrude and options.sampleFactor as separate parameters, and not properties of options.

This likely needs a change in utils/convert.js.

[IIITM-Jay]

Hi @davepagurek ,

I looked at the code to find out how the parsing logic is being written and I found that the function getParams(entry) is indeed not filtering out nested parameters like options.extrude, which should be part of options, not separate entries.

The issue lies in the line here:

return (entry.tags || []).filter(t => t.title === 'param').map(node => {
    const param = entry.params.find(param => param.name === node.name);
    ...
  });

A very heuristic approach would be like,

.filter(t => t.title === 'param' && !t.name.includes('.'))

Of course, it will work, but then it is not based on structure, it will just be based on assumptions of how naming is being done. What I meant is, it will become a bold statement as - "If the parameter name contains a (.) (like options.extrude), we assume it’s nested, so skip it."

Instead, we can use, a set to build all the top-level parameter names defined in entry.params. This will allow fast lookups when checking if a @param tag name is a top-level parameter in constant-time O(1) using .has(...) check.

const signatureParamNames = new Set((entry.params || []).map(param => param.name));

and then

.filter(t => t.title === 'param' && signatureParamNames.has(t.name))
 .map(node => {
      const param = (entry.params || []).find(param => param.name === node.name);

I have also observed two other notable things in the existing function:

1. The entry.params is checked without fallback:

const param = entry.params.find(param => param.name === node.name);

We should safely access entry.params in case it's missing (i.e., undefined). If entry.params doesn’t exist (e.g., incomplete doc output), using entry.params.find(...) without fallback would throw a runtime error. Fallback to [] ensures find(...) doesn’t break the loop.

2. The if - else block as below could be optimized using nullish fallback pattern (||) with optional chaining (?.) as they doesn't have that much differences and are similar too.

if (param) {
      return {
        ...node,
        description: param.description
      };
    } else {
      return {
        ...node,
        description: {
          type: 'html',
          value: node.description
        }
      };
    }

optimized one:

return {
        ...node,
        description: param?.description || {
          type: 'html',
          value: node.description
        }
      };

@ksen0 and @limzykenneth , would also love to hear your thoughts and feedback here.
Thanks!

[IIITM-Jay]

I have checked it too with the changes as discussed above, it is coming as expected:

 "textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "Object?"
        ]
      ]
    }

and yet another example with the below params as one of the overloads of createModel:

/**
   * @method createModel
   * @param  {String} modelString
   * @param  {String} [fileType]
   * @param  {Object} [options]
   * @param  {function(p5.Geometry)} [options.successCallback]
   * @param  {function(Event)} [options.failureCallback]
   * @param  {boolean} [options.normalize]
   * @param  {boolean} [options.flipU]
   * @param  {boolean} [options.flipV]
   * @return {p5.Geometry} the <a href="#/p5.Geometry">p5.Geometry</a> object
   */

it is coming as:

[
          "String",
          "String?",
          "Object?"
]

[IIITM-Jay]

Hi @davepagurek , @ksen0 , @limzykenneth ,
if there is a consensus on this and you folks feel that this is the correct way to approach the issue, I would be more than happy to proceed ahead raising a PR. Will revise the PR too as per the feedback and changes as required.
Thanks!

[davepagurek]

Hi @IIITM-Jay! I think there are two things to address here. The first thing is to not treat nested properties like separate positional parameters, but the second is the ability to check those types at runtime. Because we need that second case, we will need to do more than filter out the nested parameter types -- I think we'll have to pass that data into FES still. So the idea would be to turn the textToModel overloads from this:

 "textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "Object?"
        ]
      ]
    }

...to something like this (as an example syntax):

 "textToModel": {
      "overloads": [
        [
          "String",
          "Number",
          "Number",
          "Number",
          "Number",
          "{ extrude: Number?, sampleFactor: Number?  }?"
        ]
      ]
    }

...and then parse that out and check it in FES here:

p5.js/src/core/friendly_errors/param_validator.js

Line 143 in a46490d

fn.generateZodSchemasForFunc = function (func) {

(Another possible approach to avoid stringifying and then re-parsing could be to change the parameterData.json format to include more structure, similar to what we see in the data.json output of convert.mjs.)

To do so, I think you'll have to identify which properties are nested properties. We don't need to worry too much about efficiency since this is only run once at compile time (end users of p5.js don't incur that cost, just us when building the library) so we can favour simplicity for now.

There are two ways, it seems, for us to get nested parameters:

1. As you mentioned, properties with a . in the name. Checking for the existince of a . in the string is probably sufficient for now.
2. Having a property type that comes from a @typedef. I'm not sure we have any of these yet, so we can maybe ignore them for now?

[IIITM-Jay]

Hi @davepagurek! Thanks for the insights on this. I am really enjoying this as it looks like I again got something to dive deeper and playaround with the approaches for a suitable one.

... so we can favour simplicity for now.

As you mentioned, properties with a . in the name. Checking for the existince of a . in the string is probably sufficient for now.

I agree that sometimes simplicity is favoured over the optimization for easier maintenance and enhanced readability. So, I guess yes the very first approach that came to my mind as I mentioned earlier and you also recognized will suffice for filter out the nested properties.

Will work around and come back with the syntax changes as well checking it with FES.

Having a property type that comes from a @typedef. I'm not sure we have any of these yet, so we can maybe ignore them for now?

Yeah! I have checked so far, @typedef is not present yet for now.

[davepagurek]

Thanks for checking for @typedefs!

As far as PRs go, feel free to work on one if you're interested in taking this on! If tackling the problem in two stages helps, your initial idea of just removing the object properties from the array in parameterData.json could be a great first step as it prevents FES from mistakenly checking other positional properties. Changing the Object? to something with more information about its properties could be done as an additional PR if you'd like. Or feel free to tackle both issues at once if you feel inspired 🙂

[IIITM-Jay]

...to something like this (as an example syntax):

I am thinking to have a syntax like:

"textToModel": {
  "overloads": [
    [
      "String",
      "Number",
      "Number",
      "Number",
      "Number",
      {
        "type": "Object",
        "optional": true,
        "properties": {
          "extrude": "Number?",
          "sampleFactor": "Number?"
        }
      }
    ]
  ]
}

I have given an additional key as optional with boolean values to indicate that this parameter would be optional.
@davepagurek would this be ok for syntax

Or feel free to tackle both issues at once if you feel inspired

I am thinking to, let's see