Fix TypeScript typing for filterColor shader hook#8644
Fix TypeScript typing for filterColor shader hook#8644Kathrina-dev wants to merge 10 commits intoprocessing:dev-2.0from
Conversation
35fba48 to
bd57864
Compare
bd57864 to
a5f85a7
Compare
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for taking this on @Kathrina-dev! While the original issue was for filterColor, I think this actually applies to all the hook objects documented around filterColor in the code. While we could add a patch method for each one, this seems likely to drift from the documented properties, especially as we add new hooks. It might be worth instead taking the approach of updating the typescript generation script to work with JSDoc typedefs (or something else, like documenting a pretend class for each hook object or something?) so that we can have the jsdoc be the sole source of truth for both documentation and types.
|
Thanks for the suggestion, I've already removed the formatted part of the code to make it easier to identify the diff. My initial approach was to use Typescript generation script (@Property, @typedef) but it kept collapsing the JSDoc typedef into an object. I agree that adding a patch would cause a drift from the documented properties, so I'll try the alternative way you've suggested and let you know the results. |
|
Hi! If you run Looks like the way the existing typedefs are being exported has changed -- the typedef itself should be a type, but also there should be a const of the same name from the rest of the jsdoc. |
|
I think the issue was that some constants (like BLEND, ADD, etc.) weren’t being picked up properly. I updated the lookup to include them and adjusted how they’re handled in the type conversion depending on where they’re used. That seems to fix the errors. Thanks for being patient with me, I'm new to contributing to Open Source and forgot to run the test cases before committing. I'll keep it in mind next time. |
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for making some updates, good to see the tests passing now! It looks like although they past, the generated type files still aren't 100% correct. I left some comments, one of which includes some steps I'd follow when working on this to help verify the change.
| function generateTypeDefinitions() { | ||
| let output = '// This file is auto-generated from JSDoc documentation\n\n'; | ||
|
|
||
| const strandsMethods = processStrandsFunctions(); |
There was a problem hiding this comment.
Any reason why this was moved up here from below?
| constant.type.name === constant.name | ||
| ) { | ||
| // Self-referential constant → fallback | ||
| type = 'number'; |
There was a problem hiding this comment.
I think this is not sufficient yet. When I run npm run generate-types, and then open types/p5.d.ts, I see this:
declare const P2D: number;
declare const __P2D: typeof P2D;Previously, it looked like this:
declare const P2D: 'p2d';
declare const __P2D: typeof P2D;I think a next step should be not just to get the tests to pass -- first, I would git checkout dev-2.0, run npm run generate-types, and then make a copy of types/p5.d.ts. Then, if you git checkout fix-filterColor-object-type again, the goal will be to make it so that when you run npm run generated-types, and you compare the newly generated file with the old one via diff previousTypesFile.d.ty types/p5.d.ts, you should only see the addition of the FilterColorHook type.
| * @property {any} canvasSize | ||
| * @property {any} texelSize | ||
| * @property {any} canvasContent | ||
| * @property {function(): undefined} begin |
There was a problem hiding this comment.
Should these be : void too or is there a reason for them to be : undefined?
Resolves #8574
Changes
Fixes incorrect TypeScript typing for the
filterColorshader hook.Previously, the generated TypeScript definitions declared:
declare const filterColor: object;
Because of this, accessing documented properties such as:
filterColor.texCoordfilterColor.canvasSizefilterColor.begin()filterColor.set(...)resulted in TypeScript errors like:
Property 'texCoord' does not exist on type 'object'.
Other Methods attempted at resolving the error
Initially I attempted to resolve this through JSDoc annotations in the source by using:
@property@typedefto explicitly define the structure of
filterColor.However, the documentation generator used in the build pipeline still produced the type as
objectin the generated TypeScript definitions. Because of this limitation in the JSDoc → TypeScript generation step, the correct structure was not preserved.Solution
To ensure the generated typings match the documented API, I added a patch in:
utils/patch.mjs
This patch updates the generated declaration to:
This aligns the TypeScript definitions with the documented properties of the
filterColorshader hook and prevents TypeScript errors when accessing them.Screenshots of the change
N/A (TypeScript typing fix)
Suggested Reviewers
PR Checklist
npm run lintpasses