Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnimStateGraph: Add JSDoc types #2986

Closed
wants to merge 4 commits into from

Conversation

kungfooman
Copy link
Collaborator

@kungfooman kungfooman commented Mar 19, 2021

Currently there are no types in playcanvas.d.ts:

image

This PR implements all the types the class AnimStateGraph accepts:

image

This enables Intellisense for e.g. PC Viewer:

image

Fixing ESLint is not so easy, because I am running into some VSCode/TypeScript issue or so: eslint/eslint#14235

I confirm I have signed the Contributor License Agreement.

@kungfooman kungfooman marked this pull request as draft March 19, 2021 13:25
@kungfooman kungfooman marked this pull request as ready for review March 19, 2021 15:33
@yaustar
Copy link
Contributor

yaustar commented Jun 8, 2021

@willeastcott @ellthompson is this a PR that we can review + merge?

@Maksims
Copy link
Contributor

Maksims commented Jun 8, 2021

How does this affects docs generation?
Also, this is something we haven't done before, and looks like specifically for TypeScript, not for everyone else.

@kungfooman
Copy link
Collaborator Author

How does this affects docs generation?

Good point, it currently produces a 404, which should be fixed to produce a proper object description (maybe inline):

image

Also, this is something we haven't done before, and looks like specifically for TypeScript, not for everyone else.

I mainly did this for playcanvas-viewer, which is written in TypeScript, to get rid of // @ts-ignore cases. It also helps JavaScript developers in Visual Studio Code or any other editor that supports JavaScript IntelliSense.

@vmwxiong
Copy link
Contributor

vmwxiong commented Jun 8, 2021

How does this affects docs generation?
Also, this is something we haven't done before, and looks like specifically for TypeScript, not for everyone else.

Eavesdropping in, but I'd love to see support for explicit TS definition contributions somehow. Been doing a bunch of work to get proper types for some things, but not sure how I could possibly contribute the work to the project at all: https://gist.github.com/vmwxiong/eabc60834b32801f5a38de99f4acad80

@kungfooman
Copy link
Collaborator Author

Eavesdropping in, but I'd love to see support for explicit TS definition contributions somehow.

The generated playcanvas.d.ts is kinda as "proper" as it gets, only that generating it through JSDoc feels a bit convoluted. The custom doc generation from it doesn't make it more straight forward either, like fixing something in this repo needs another fix in https://github.com/playcanvas/playcanvas-jsdoc-template e.g.

For more "advanced" typing like yours it would make sense to append it directly to playcanvas.d.ts after its generation, so it's shared for anyone who installed PlayCanvas through npm. Putting extra typing clues in too many places could make it overly confusing though, so one should try to make use of JSDoc whenever possible (and keep playcanvas-jsdoc-template synched to display types in an informative/easy/understandable way)

Copy link

@hixio-mh hixio-mh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviews and change

@kungfooman
Copy link
Collaborator Author

Any is good enough, we shouldn't overcomplicate types like this.

@kungfooman kungfooman closed this Nov 11, 2021
@vmwxiong
Copy link
Contributor

any is extremely painful to with with as it blows up attempts at building type-safe code, and you need to write and use convoluted type guards every time you want to use something typed with an any value.

Disappointed to see the project not moving to support type safe definitions.

@willeastcott
Copy link
Contributor

@vmwxiong I think you missed this conversation: playcanvas/model-viewer#98 (comment)

The plan is to make AnimStateGraph internal. So this is no longer an issue.

You may have also noticed this week that I have submitted several issues on the repo about improving the Typescript definitions. So saying that 'the project is not moving to support type safe definitions' is not fair or accurate, IMHO. The only time it is difficult to solve typing definitions problems is when it negatively impacts the quality of the API reference manual. Otherwise, everybody is excited to be improving the quality of the typings.

@vmwxiong
Copy link
Contributor

vmwxiong commented Nov 11, 2021 via email

@kungfooman kungfooman mentioned this pull request May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants