Skip to content
This repository has been archived by the owner on Jun 16, 2024. It is now read-only.

Support typedefs #60

Closed
wants to merge 2 commits into from
Closed

Conversation

kungfooman
Copy link
Contributor

I wanted to do this for a long time 😅

This allows us to finally declare a typedef like:

/**
 * Animal counting object for a zoo.
 *
 * @typedef {object} AnimalCount
 * @property {number} elephant - The number of elephants.
 * @property {number} dolphins - The number of dolphins.
 */

/**
 * @param {AnimalCount} animalCount - Object containing counts of animal groups.
 * @returns {number} Number of animals.
 */
export function countAllAnimals(animalCount) {
    return animalCount.elephant + animalCount.dolphins;
}

And it will be exposed like this in docs/pc.html:

image

If you click on it you see the full definiton:

image

Having a function that accepts a typedef will also lead you to the correct definition, similar to this scenario:

image

My main motivation is a type issue I found in the engine, basically both VertexIteratorAccessor and VertexFormat define what a vertex element is, but with latest Splat changes we suddenly have one definition differing from the other (there is a new asInt property).

(...there are more cases we can use typedefs for to create a better playcanvas.d.ts)

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@Maksims
Copy link
Collaborator

Maksims commented Dec 4, 2023

This achieves a very similar to the current approach of using multiple parameter with dot-notation on specifying object structure.
Current approacb keeps the docs of it where it is used, not separating important info to somewhere else. And does not pollute autocomplete or docs with too many abstract objects.

Example of using current params with objects:
https://github.com/playcanvas/engine/blob/main/src/framework/xr/xr-manager.js#L370

This seems like doing almost the same, but it will require extra navigation in docs to read actual params - which is not good. Same applies to the code: reading sources with docs where it is relevant is better, instead of navigating to some other file or line in the file to read it.

If it is a common passable object around, it worth a class and that is the way currently.

@willeastcott
Copy link
Collaborator

What does this do to the TypeDoc output (which we'll switch to in the not too distant future)?

@kungfooman
Copy link
Contributor Author

What does this do to the TypeDoc output (which we'll switch to in the not too distant future)?

Currently it looks rather anemic, I opened an issue about it: TypeStrong/typedoc#2454

image

They seem to parse the complete type information here though - so I'm not sure if this is just a template issue or something:

TypeStrong/typedoc@1153735

So right now it looks just as anemic as callbacks in typedoc:

image

@kungfooman
Copy link
Contributor Author

kungfooman commented Dec 5, 2023

@Maksims Yes, I see your points, but you cannot substitute typedefs with object-with-dot-notation parameters.

Mathematically speaking, object-with-dot-notation parameters are only a subset of typedefs.

You also cannot substitute typedefs with classes for "fake typing" in all cases, for example I reported bugs in the engine that stem from JSON data not being typed. I use old assets and their JSON data lack all kinds of new properties. So instead of something being boolean, it suddenly is implicitly false and the only reason I find these issues is that I cram for hours through non-existing types.

So personally I find it more efficient to simply click on a link and receive full type information in docs while having no errors in the engine than a multitude of bugs and hours of debugging, because... people can't handle a typedef in docs/pc.html?

BTW I'm not advocating for replacing object-with-dot-notation parameters with typedefs, there are simply instances that object-with-dot-notation parameters cannot cover.

The reality in the engine right now is simply... type bugs:

image

And I feel rather dedicated to fix them all, one by one... a bit of trust in the process 🙏

@Maksims
Copy link
Collaborator

Maksims commented Dec 5, 2023

BTW I'm not advocating for replacing object-with-dot-notation parameters with typedefs, there are simply instances that object-with-dot-notation parameters cannot cover.

Makes sense. This probably worth an information in CONTRIBUTING.md to ensure developers have good guideline where object-with-dot-notation is preferred (e.g. constructor with passing one-off objects) and where typedef is preferred (common objects passed between different methods/classes).

@kungfooman
Copy link
Contributor Author

kungfooman commented Dec 5, 2023

This probably worth an information in CONTRIBUTING.md

Good point 👍

I have possibilities in mind that can act as good example when the time is ripe for it 👀

@kungfooman
Copy link
Contributor Author

Very nice, we can fix callbacks and typedefs via this plugin:

npm install typedoc-plugin-missing-exports --save-dev

And changing engine/package.json:

"typedocs": "typedoc --plugin typedoc-plugin-missing-exports",

Now it looks like this:

image

image

(big thank you to @Gerrit0)

@willeastcott
Copy link
Collaborator

Closing since we have now moved to TypeDoc.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants