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

Add field declarations for all core classes #5033

Merged
merged 2 commits into from Feb 2, 2023

Conversation

willeastcott
Copy link
Contributor

This PR adds field declarations for all classes in the src/core folder. This improves the Typedocs, changing this:

image

Into this:

image

It also further aligns the code with TypeScript, where field declarations are mandatory.

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

@willeastcott willeastcott requested a review from a team February 2, 2023 18:42
@willeastcott willeastcott self-assigned this Feb 2, 2023
@willeastcott willeastcott merged commit e7f56be into main Feb 2, 2023
@willeastcott willeastcott deleted the core-field-declarations branch February 2, 2023 19:27
@kungfooman
Copy link
Collaborator

You are rolling through this like a champ, very nice to see the progress 👍 🎯

May the JSDoc shackles be forgotten at some point 😅

@willeastcott
Copy link
Contributor Author

Wouldn't that be great? Have you inspected the Typedoc output yet? What do you think of the styling and layout compared to the JSdoc output?

@kungfooman
Copy link
Collaborator

kungfooman commented Feb 3, 2023

So far I can see some pros and cons, lets just take one example to make it specific:

image

image

I think the presentation of a factual data in TypeDoc is:

  1. overly "typified", which makes it rather confusing to read
  2. redundant
  3. taking too many new lines, which creates a lot of "air" inbetween
  4. numVerts??: number is simply ugly compared to [numVerts]
  5. the addiction to : everywhere, creating ASCII soup that brain/eyes need to filter away

typedoc signature: computeMinMax(vertices: number[] | Float32Array, min: Vec3, max: Vec3, numVerts??: number): void

While in JSDoc there is a short line that looks like something you can copy straight into the source code with minimal changes:

JSDoc signature: computeMinMax(vertices, min, max, [numVerts])

JSDoc presents what I really want to see/copy/use, in a short line.

Another example:

image

"returns a BoundingBox Returns Returns BoundingBox" 🤔

image

It nearly feels like 3 or 4 times as big as it "really" needs to be. They somehow got addicted to putting that : everywhere, as if that helps.

What I like about TypeDoc is the link to the actual source code.

Or the search just feels better:

image

JSDoc finds nothing:

image

So my feeling is that typedoc is technically more "aware" of certain things, but the representation absolutely needs to be improved. While I like JSDoc for its simple presentation of exactly what I want to see.

Hm, I thought that typedoc supports @typedef's but it doesn't 😢

/**
 * Super typey type.
 * 
 * @typedef {object} SuperTypeyType
 * @property {number} a - The a.
 * @property {number} b - The b.
 */

image

Feels still better in JSDoc, even tho it's a 404 that would need to be created:

image

Edit: What do you think about displaying all inherited methods in every class? JSDoc filters them, which I think makes it shorter and displays what's "important" for the actual class, making it more readable 🤔

@willeastcott
Copy link
Contributor Author

@kungfooman This is great feedback. I get what you say about the verbosity of the Typedoc output. But I guess anyone developing against TypeScript are going to love it! 😄

At the moment, the Typedoc output is highly experimental, and of course, it just uses the default template for now. It is no doubt customizable (although I don't know to what extent). I think I'll continue to experiment as see where it goes....

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

3 participants