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

Change default original event type from undefined to unknown #2243

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

neodescis
Copy link
Collaborator

@neodescis neodescis commented Mar 3, 2023

Launch Checklist

Currently, the default type for originalEvent on a MapLibreEvent is "undefined." This can lead to type errors when handling some events unless the type for originalEvent is specified on the handler, even when we may not care about the type of orginalEvent at all. For example, let's say I want to handle a zoom event. If I define an event handler like this:

onZoom(event: MapLibreEvent) {}

I will get a type error if I try to subscribe to a zoom event using that callback, because that event is typed as MapLibreEvent<MouseEvent | TouchEvent | WheelEvent | undefined>. The TypeScript compiler complains about the three non-undefined event types being unassignable to undefined.

Moreover, the type "unknown" is more correct here anyway, as the base case is not knowing what the type is. In the case where the handler doesn't care about originalEvent, using unknown as the default instead means you don't have to provide the type at all.

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Add an entry to CHANGELOG.md under the ## main section.

@HarelM
Copy link
Collaborator

HarelM commented Mar 3, 2023

Interesting. Thanks for taking the time to open this PR.
Can you think of a test to cover this change?

@neodescis
Copy link
Collaborator Author

neodescis commented Mar 3, 2023

Well, I guess that depends on what you mean by "test." This change, being purely a typing change, would not alter how anything actually runs, and it also wouldn't affect the way any tests would run. It does affect compilability though, so I could probably add a test that would build with this that wouldn't build before. It would be a simple event subscription, just like the above. Is that what you're looking for?

@HarelM
Copy link
Collaborator

HarelM commented Mar 3, 2023

Yes, I think so. Or even changing an existing test to benefit from the change you just made.

@neodescis
Copy link
Collaborator Author

Test added!

@HarelM
Copy link
Collaborator

HarelM commented Mar 5, 2023

THANKS!!

@HarelM HarelM merged commit 72a9fed into maplibre:main Mar 6, 2023
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