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

Typing events tile renderer #865

Merged
merged 3 commits into from
Dec 22, 2024

Conversation

sguimmara
Copy link
Contributor

@sguimmara sguimmara commented Dec 12, 2024

This PR adds types for events raised by TilesRenderer.

Note: the various methods for the event dispatcher are copy-pasted from the EventDispatcher type in @types/three. If you know a way to avoid duplicating the methods, I'd be happy to use it instead.

Note 2: I also replaced the Number and Boolean types with their primitive equivalent number and boolean (lowercase), as recommended by Typescript:

The type names String, Number, and Boolean (starting with capital letters) are legal, but refer to some special built-in types that will very rarely appear in your code. Always use string, number, or boolean for types.

@sguimmara sguimmara marked this pull request as draft December 12, 2024 08:49
@sguimmara
Copy link
Contributor Author

Reverting to draft as there are unwanted diff due to incorrect formatting

@sguimmara sguimmara force-pushed the typing-events-tile-renderer branch from ed463e6 to f3b64fd Compare December 12, 2024 08:55
@sguimmara sguimmara marked this pull request as ready for review December 12, 2024 08:55
@gkjohnson
Copy link
Contributor

Thanks again for the type additions - I'm not really familiar with typescript so these template types look really complicated to me. If you have brief explanation that would be great but either way if types like this are going to be added to the project I'm going to have to ask for some commitment to helping to answer questions or maintain them. Is that something you or the Giro3D project can help with?

Also the event system isn't just limited to the events dispatched from within the class. It's possible for end users or plugins to add their own event types for their own uses:

tiles.dispatchEvent( { type: 'custom-tiles-event' } );

The TilesFadePlugin does this, for example. I'm not sure if it's possible to add this kind of flexibility to the event system as well (basically allow for an "any" event).

@sguimmara
Copy link
Contributor Author

Thanks again for the type additions - I'm not really familiar with typescript so these template types look really complicated to me. If you have brief explanation that would be great but either way if types like this are going to be added to the project I'm going to have to ask for some commitment to helping to answer questions or maintain them. Is that something you or the Giro3D project can help with?

Also the event system isn't just limited to the events dispatched from within the class. It's possible for end users or plugins to add their own event types for their own uses:

tiles.dispatchEvent( { type: 'custom-tiles-event' } );

The TilesFadePlugin does this, for example. I'm not sure if it's possible to add this kind of flexibility to the event system as well (basically allow for an "any" event).

You're right, there might be a way of doing it with arbitrary plugins, by unioning the event types of all plugins + the base object. I'll see if I can find a way that is ergonomic enough. Otherwise, I'll abandon this PR.

I'll also open a separate PR for the removal of wrapper types (i.e Number-> number), because it makes sense to do it once for all the typescript definition files rather than one file.

@sguimmara sguimmara marked this pull request as draft December 14, 2024 11:17
@gkjohnson
Copy link
Contributor

You're right, there might be a way of doing it with arbitrary plugins, by unioning the event types of all plugins + the base object.

I don't necessarily mean that we need to include all the plugin events explicitly as much as the dispatch and event listen functions need to be flexible enough to allow for users to pass in event types that are not included in the d.ts file since end users may make their own custom events in their own plugins, as well.

I appreciate the work on this!

@sguimmara sguimmara force-pushed the typing-events-tile-renderer branch 3 times, most recently from f0eb1e9 to b386c4c Compare December 16, 2024 13:06
@sguimmara sguimmara force-pushed the typing-events-tile-renderer branch from b386c4c to 7f50a26 Compare December 21, 2024 11:15
@sguimmara sguimmara force-pushed the typing-events-tile-renderer branch from 7f50a26 to 09ee3f3 Compare December 21, 2024 11:59
@sguimmara
Copy link
Contributor Author

@gkjohnson I updated the PR with the following changes:

  • added a way to test the API files (*.d.ts) without actually running test (just using the typechecker). By calling the typescript compiler on the test files, we can detect API breaking changes (for example, an event argument that was removed or renamed, or its type changed)
  • fixed incorrect class name for the ReorientationPlugin

@sguimmara
Copy link
Contributor Author

You're right, there might be a way of doing it with arbitrary plugins, by unioning the event types of all plugins + the base object.

I don't necessarily mean that we need to include all the plugin events explicitly as much as the dispatch and event listen functions need to be flexible enough to allow for users to pass in event types that are not included in the d.ts file since end users may make their own custom events in their own plugins, as well.

I appreciate the work on this!

The event dispatcher supports both typed and unknown events, that is why there are two versions of event dispatcher methods: one for types that are known (and the type signature of their event handler known as well), and the events that are unknown, such those dispatched by plugins. For the latter, the event listener is not typechecked.

@sguimmara sguimmara marked this pull request as ready for review December 21, 2024 12:06
Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Okay, thanks! I'll merge this but I'll have to say I don't fully understand the typing here so as I've mentioned before I'll have to leave it to others to properly maintain some of these types.

Comment on lines +6 to +9
"include": [
// Type-based test files to check API
"test/types/"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: looks like this is invalid json and indentation looks off

@gkjohnson gkjohnson merged commit bdea72b into NASA-AMMOS:master Dec 22, 2024
2 checks passed
@gkjohnson gkjohnson added this to the v0.3.46 milestone Dec 22, 2024
@sguimmara sguimmara deleted the typing-events-tile-renderer branch December 24, 2024 16:36
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.

2 participants