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

Incorrect TypeScript declarations

Open Toby222 opened this issue 4 years ago • 5 comments

World

The World constructor's parameters are marked in a JSDoc comment in World.js as follows, and correctly handles getting passed a Vec2 as setting gravity.

/**
 * @param {WordDef|Vec2} def World definition or gravity vector.
 */
function World(def) {
 ...

World's constructor in index.d.ts is marked as below and will cause warnings/errors when passed a Vec2 as parameter

export class World {
 constructor(def?: WorldDef);
 ...

Vec2

Various places list needing Vec2 but will also take a simpler Object in the form of { x: number, y: number }

Toby222 avatar Feb 06 '21 16:02 Toby222

Thanks for pointing that out! Origininally the constructor was typed as

new(def?: WorldDef | Vec2 | null): World;

but somehow that got lost in the typings. @shakiba Do you know if that was an intentional change?

You are right that { x: number, y: number } is sometimes enough but I would still prefer to use Vec2 here if thats ok for you. Otherwise we would get inconsistent typings because of implementation details (like why I have to use Vec2 in method A and not in method B) and you can still call it using the simpler object by World(Vec2(obj))

zOadT avatar Feb 07 '21 12:02 zOadT

also I just noticed that apparently it says WordDef instead of WorldDef? At least I think copy-pasted it

Toby222 avatar Feb 07 '21 18:02 Toby222

For the Vec2, I also think it is better to keep it consistent to avoid confusion about which methods accepts x-y or require Vec2. However accepting x-y is helpful for interactions between the library and rest of code, so if it is possible to update all public methods to accept x-y it would be great (I don't remember which methods accept x-y and which ones don't). We can create an interface for x-y which Vec2 implements too.

I will fix the world today, just one question for @zOadT: does following code work for world, because I remember something similar didn't work for shapes:

export function World(def?: WorldDef | Vec2 | null): World;
export class World {
  constructor(def?: WorldDef | Vec2 | null);
}

shakiba avatar Feb 07 '21 21:02 shakiba

I did a quick test and it works as expected 👍 (I don't exactly remember the problem for shapes, just know that extending a class did not work in declaration files)

BTW: technically you are not required to let Vec2 implement x-y because TS detects that automatically (But of course it may be more readable if you explicitly implement it)

zOadT avatar Feb 08 '21 07:02 zOadT

Cool, thanks! I pushed the fix, will be in next release.

shakiba avatar Feb 08 '21 07:02 shakiba