paperjs / paper.js

The Swiss Army Knife of Vector Graphics Scripting – Scriptographer ported to JavaScript and the browser, using HTML5 Canvas. Created by @lehni & @puckey
http://paperjs.org
Other
14.45k stars 1.22k forks source link

Some missing types of build-in typescript definition file. #1659

Closed Domvel closed 5 years ago

Domvel commented 5 years ago

Since version 0.12.1 and upcoming major/minor release there are build-in typings for TypeScript. #985

Heres my post from this issue:

It seems to work generally. The typed-file is detected and works for import. But a few definitions are missing. Here just a few examples from my project.

import * as paper from 'paper';
// Missing setup-method in global scope of paper namespace. also install-method.
paper.setup(canvas);
// Quick'n'Dirty Workaround
(paper as any).setup(canvas);

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5344bfc80508c53a23dae37b860fb0c905ff7b24/types/paper/index.d.ts#L66

It works with setup() but maybe I should use it in another way? But setup() exists.

// Argument of Path is only a "object" not a interface with defined properties.
// It's ok, no problem, but a weak type. e.g. no autocompletion / suggestion in vs code :(
// Note: Not all properties are required. So they should be marked as optional typescript: `?`
const path = new paper.Path({
  strokeColor: '#ffff00',
  strokeWidth: 5,
  strokeCap: round
});

// Throws an error: Argument of type `number[]` is not assignable to parameter of type `Point | Segment`.
// But this is not true. It is possible to assign number[].
path.add([100, 200]);
// Quick'n'Dirty Workaround :(
(path as any).add([100, 200]);

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5344bfc80508c53a23dae37b860fb0c905ff7b24/types/paper/index.d.ts#L2418

That is interesting. The types/paper definition file also has only Segment | Point. But for sure number[] also works. I've tested it. The types/paper throws no error.

Reference: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5344bfc80508c53a23dae37b860fb0c905ff7b24/types/paper/index.d.ts

The latest (paper documentation)[http://paperjs.org/reference/global/] also has no setup-method. Don't know if the last documentation had it. I think it's just a missing declaration in the docs / comment. Update: Wait... there it is: http://paperjs.org/reference/paperscope/ But setup() also exists in global scope. 🤔 Is this maybe a predefined paperscope? It seems so, yes. This means setup and install is missing in the types.

Some versions. (package.json)

{
  "paper": "0.12.1",
  "typescript": "3.2.4"
}

VS-Code Info incl. OS, Node, etc.

Version: 1.35.0 (user setup)
Commit: 553cfb2c2205db5f15f3ee8395bbd5cf066d357d
Date: 2019-06-04T01:17:12.481Z
Electron: 3.1.8
Chrome: 66.0.3359.181
Node.js: 10.2.0
V8: 6.6.346.32
OS: Windows_NT x64 10.0.17763

Any questions? I'm glad if I can help.

lehni commented 5 years ago

@sasensi these are all either pointing towards missing JSDoc comments for existing methods / properties, or then missing functionality in the converter that you've created that may not take all versions of function / constructor signatures into account? For the docs, we added quite a few extensions to the JSDoc standard back then, happy to explain them to you if it's not clear. Could you look into this and see what could be improved?

sasensi commented 5 years ago

@Domvel, thanks for the feedback. @lehni, yes sure, there is a lot of space for improvement there and I was aware of that when developing this feature. I only went to the point to make it work without changing too many things. I am glad to see people interested in this feature and I will be glad to improve it too. That said, there is a decision to make on how far we want to go because typescript is made to provide complex and deep type information but JSDoc is far less advanced (at least the JSDoc version that we are using). Paper.js also have a lot of hidden APIs like path.add([number, number]) mentioned above which are not documented and that seems to be cumbersome to cover them all in JSDoc context. In the extreme case where we would want to cover all possible signatures, including optional or required properties..., we would be better using typescript itself as the base for our doc, but we would lose the coupling between the code and the doc... I think that there is a balance to find.

I see multiple possible ways to go:

@lehni, what do you think ?

Domvel commented 5 years ago

A few days ago I thought about rewriting this in TypeScript or the latest JavaScript (ES6+) with Webpack etc. I like JavaScript, I love TypeScript. It is cleaner and more scalable. And you write fewer comments because it can be derived from the source code. The generation of documentation also works great / better because more consistent. You can use references in the editor such as VSCode and less error prone. etc. ... but I checked the source code a bit. And I think a port to TypeScript will be very difficult. This is not a criticism of the code or your great work. It's just a general problem with JavaScript and its freedom. ...

I just want to know what you think about TypeScript for paper.js. Disgusted or interested?

In my opinion, converting paper.js is not impossible, but it will be a lot of work. Realistically this will never happen. And that should not be an attempt to persuade anyone. Maybe TypeScript comes for paper.js 2.0. But the best solution for now is to complement the comments / docs and complete typescript tests. Please correct me if I'm wrong.

lehni commented 5 years ago

@sasensi some thoughts on this:

lehni commented 5 years ago

As for the way forward, I think since the paper.js codebase itself is quite aging and should probably be fully replaced / rewritten with something modern quite soon, I don't think now is the time to replace JSdoc. It's probably better to wait until we start rewriting the classes in ES6, if we ever get around to that? Currently I would favour the incremental / small improvements approach. This doens't mean we can't keep changing our forked version of JSDoc and add other tags / features to it, as we've done in the past.

@sasensi what do you think?

sasensi commented 5 years ago

@lehni, thanks for the answer.

dobesv commented 5 years ago

The more I look at this issue, the more it seems impractical to autogenerate the typescript files from the JSDoc comments that are there now. It would be easier to maintain the typescript files by hand, as has been done in the past using @types/paper.

One option might be to just embed full typescript declarations into the JSDoc. Then if someone updates the docs they will be more likely to also update the typescript spec. And they could be included in the docs as a kind of more rigorous spec.

You could also generate a typescript test file that ensures that the typescript specs covers whatever is in the JSDocs (at a minimum). Then have your continuous integration run this test case (and maybe some manually added ones) to ensure the typescript specs are more or less complete. Continue to manually update the typescript (whether in a standalone file or embedded into JSDocs) so you can cover the odd cases that are too hard to capture with JSDoc comments.

sasensi commented 5 years ago

@dobesv, thank you for your ideas. Maybe you are right and auto-generating typescript definition from JSDoc was not a good idea.

And at the same time, I see many benefits in it:

So I think that it is just a matter of finding a good way to document rather complex things but once done, we will benefit from it, without further maintaining work needed. And I am totally open to working on those improvements.

Furthermore, I think that half of the issues that were reported these last days are not linked to the fact that we are auto-generating the typescript definition but rather to the nature of typescript itself and its compatibility issues with the library. I speak about nullable properties issue: https://github.com/paperjs/paper.js/issues/1664 and getter and setter accepting different types issue: https://github.com/paperjs/paper.js/issues/1665. So for those ones, going back to a manually generated definition won't change anything. For the others, I already did fixes that I will push soon.

HriBB commented 3 years ago

In my project I had some errors, when typescript did not recognize function signature. I was able to "extend" the global paper namespace like this:

declare global {
  namespace paper {
    export interface View {
      scale(scale: number, center?: paper.Point): void
      scale(hor: number, ver: number, center?: Point): void
      scale(scale: number, center?: [number, number]): void // added
      translate(delta: paper.Point): void
      translate(dx: number, dy: number): void // added
    }
    export interface Raster {
      fitBounds(rectangle: Rectangle, fill?: boolean): void
      fitBounds(x: number, y: number, width: number, height: number): void // added
    }
    export interface Tool {
      view: View;
    }
    export interface ToolEvent {
      event: MouseEvent;
      tool: Tool;
    }
  }
}

Note that you need to copy paste all methods with the same name then add custom one in the override.

Maybe we could add something like this to the project?

sasensi commented 3 years ago

Indeed, your workaround make me think that we could somehow find a way to replace class instances by their scalar equivalent (like you did for Point in the scale method).
Another maybe less verbose approach though could be to use union type instead of signature overrides:

scale(hor: number, ver: number, center?: (Point | {x:number, y:number} | [number, number])): void
...
HriBB commented 3 years ago

Made a few mistakes, this is the last version I am using right now

declare global {
  namespace paper {
    interface PaperScope {
      exportJSON(): any // added
    }
    interface Project {
      exportJSON(options?: object): object[] // added
    }
    interface View {
      scale(scale: number, center?: Point): void
      scale(hor: number, ver: number, center?: Point): void
      scale(scale: number, center?: number[]): void // added
      translate(delta: Point): void
      translate(dx: number, dy: number): void // added
      projectToView(point: Point): Point
      projectToView(point: number[]): Point // added
      viewToProject(point: Point): Point
      viewToProject(point: number[]): Point // added
    }
    interface Raster {
      fitBounds(rectangle: Rectangle, fill?: boolean): void
      fitBounds(x: number, y: number, width: number, height: number): void // added
    }
    interface Item {
      selectionKey: Selection; // added
    }
    interface Tool {
      view: View; // added
    }
    interface ToolEvent {
      event: PointerEvent; // added
      tool: Tool; // added
    }
  }
}

I don't really care about the style ...

scale(hor: number, ver: number, center?: (Point | {x:number, y:number} | [number, number])): void

vs

type PointInput = Point | {x:number, y:number} | [number, number]
scale(hor: number, ver: number, center?: PointInput): void

vs

scale(scale: number, center?: Point): void
scale(hor: number, ver: number, center?: Point): void
scale(scale: number, center?: number[]): void

As long as it works and does not take much to implement and maintain :)

HriBB commented 3 years ago

How are types defined/generated for paperjs? Can you please point me into the right direction.

Would be cool if there was some section in the docs. Took me a while to realize, there's a global paper namespace I can use haha :)

sasensi commented 3 years ago

As mentioned above, types are currently generated from the code JSDoc comments + some extra specific things.
The implementation is not very easy to understand because it relies on a custom JSDoc parser which was initially made to generate the documentation but the core of it is here if you want to have a look: https://github.com/paperjs/paper.js/tree/develop/gulp/typescript

sasensi commented 3 years ago

@HriBB, I just pushed a PR adding support for arguments reading pattern, that should solve your issue.

HriBB commented 3 years ago

@sasensi cool thanks. I will check it out soon.