[p5.js 2.0 Bug Report]: Many chainable p5 instance methods are typed to return void rather than p5

[gwhitney]

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

v2.2.0-rc6

Web browser and version

n/a

Operating system

OpenSUSE Tumbleweed Linux (up-to-date)

Steps to reproduce this

Steps:

1. In a TypeScript instance-mode project, in which the variable sketch has type p5, write sketch.background(this.backgroundColor).strokeWeight(0).
2. Attempt to compile with tsc
3. Obtain the error

TS2339: Property 'strokeWeight' does not exist on type 'void'.

663         sketch.background(this.backgroundColor).strokeWeight(0)

(this happened to occur on line 663 of my code, which is not relevant).

Discussion:

The error is unsurprising, in that the p5.d.ts file generated by npm run generate-types contains the following declarations:

  background(color: p5.Color): void;
  background(colorstring: string, a?: number): void;
  background(gray: number, a?: number): void;
  background(v1: number, v2: number, v3: number, a?: number): void;
  background(values: number[]): void;
  background(image: p5.Image, a?: number): void;

But these declarations are incorrect because the implementation of background() does not return void:

fn.background = function(...args) {
      this._renderer.background(...args);
      return this;
    };

And indeed, the JSDoc comments for all of the signatures of background explicitly specify @chainable, meaning they return the object they were called on. So this appears to be a bug in generate-types; all of those void return types for background should be p5. And note this is not just on background; there are multiple other chainable methods on a p5 instance that are declared to return void, including at least (but probably not limited to, these are just the ones creating errors in our code and making it impossible to upgrade to p5.js 2.1+):

stroke, fill, textSize (with an argument), noStroke, frameRate (with an argument), translate.

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms.

For guidance on contributing, check out our contributing guidelines and other resources for contributors.
💬 If you have questions or need support, feel free to join the Processing Foundation Forum or visit our Community page.
📜 Please also review our Code of Conduct to understand our community standards.
Thank You!

[davepagurek]

This is currently actually the "correct" behaviour -- a lot of methods mark themselves as chainable, but also we have an established pattern in p5 of having a joined getter and setter -- e.g. pixelDensity(2) sets, and pixelDensity() returns the current value. While it could work to have it only be chainable on the setter variant, it does also complicate the types, and we never actually mentioned chaining in the p5 reference, so it was an undocumented API. For now, we're choosing to make the TS types match what's shown on the p5 reference, ignoring method chaining, so that we have a bit more flexibility to extend the more established joint setter/getter pattern elsewhere without it clashing with the less used chaining API. For now we do actually want you having to @ts-ignore method chaining because that behaviour may not be present in the future.

[gwhitney]

Ok, what i am hearing is "chaining on a p5 instance is deprecated". My opinion is chaining improves clarity/elegance and helps make instance mode "a bit more like" global mode, but i guess that's up to you as API designers. We will go ahead and remove p5 chaining from our code; we are certainly not going to sprinkle it with ts-expect-error comments, which would well outweigh the elegance gain of chaining.

Thanks for letting us know.