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

Refactor JSDocs for events #5974

Merged
merged 5 commits into from
Jan 24, 2024
Merged

Refactor JSDocs for events #5974

merged 5 commits into from
Jan 24, 2024

Conversation

willeastcott
Copy link
Contributor

@willeastcott willeastcott commented Jan 24, 2024

Refactor the JSDoc blocks for all events in the engine:

  • Event blocks now appear first in the class definition.
  • Events like add and add:[name] have been combined under add (as variants).
  • Each event has a static string property (this is required or tsc does not see the events). The static string is just a mechanism to document the event - the developer is not expected to use it in their code (although there's no harm in doing that).
  • Every event now has an @example tag written in modern JS.

This PR enables all events to now show in the new TypeDoc API reference manual. As an example, here's the new look of the SpriteComponent API ref page:

image

And:

image

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

@willeastcott willeastcott added the docs Documentation related label Jan 24, 2024
@willeastcott willeastcott self-assigned this Jan 24, 2024
@Maksims
Copy link
Collaborator

Maksims commented Jan 24, 2024

With replacing event string with constants, is there info on how V8 actually handles constant strings in the code?
It might be that todays V8 situation that there is very little difference in performance when using constant that points to a string vs actual string.

@willeastcott
Copy link
Contributor Author

@Maksims I don't have any information on that, but:

  • These are static (class) properties. They have negligible overhead.
  • The static strings are just a mechanism to document the events - the developer is not expected to use the EVENT_ names in their code (although there's no harm in doing that). Instead, they'll continue to use the string literals.

@kungfooman
Copy link
Collaborator

This PR enables all events to now show in the new TypeDoc API reference manual.

Will they still show in normal JSDoc (npm run docs)?

Somehow strange that we have to increase build size for docs.

@Maksims
Copy link
Collaborator

Maksims commented Jan 24, 2024

@Maksims I don't have any information on that, but:

  • These are static (class) properties. They have negligible overhead.
  • The static strings are just a mechanism to document the events - the developer is not expected to use the EVENT_ names in their code (although there's no harm in doing that). Instead, they'll continue to use the string literals.

Is there a way to make new docs respect @event or some alternative. As the name of an event in docs is also best to be same as actual event name.

This is convenient:
image

@kungfooman
Copy link
Collaborator

Is there a way to make new docs respect @event or some alternative.

We aren't the first asking for this feature: https://github.com/TypeStrong/typedoc/issues?q=jsdoc+event

Unfortunately the maintainer doesn't care about @event and just closes every issue without proper solution, so we pretty much have two options:

  1. Give in to strange ways of doing things for lack of native event support in typedoc
  2. Develop a typedoc plugin

@willeastcott
Copy link
Contributor Author

Somehow strange that we have to increase build size for docs.

TypeDoc/tsc may be strange in some limited circumstances, but if it's an overall win, I can live with a teeny tiny size increase.


/**
* Fired when a touch is canceled on the component. Only fired when useInput is true. The
* handler is passed an {@link ElementTouchEvent}.
Copy link
Contributor

Choose a reason for hiding this comment

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

so glad you preserved the types of arguments too.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

What an effort!

@kpal81xd - any events you might need to update similarly for the Gizmos?

@willeastcott willeastcott merged commit 74fbd53 into main Jan 24, 2024
7 checks passed
@willeastcott willeastcott deleted the event-docs branch January 24, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants