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

🏷️ improve addEventListener typings #2127

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Mar 31, 2023

Motivation

improve addEventListener typings

Changes

Use TS "event map" to enforce stricter types and better type inference to addEventListener and addEventListeners functions

Screenshot 2023-03-31 at 16 47 09
Screenshot 2023-03-31 at 16 47 57
Screenshot 2023-03-31 at 16 48 44

With this change, we lose the required DOM_EVENT.* usage when using addEventListener, meaning we can use "click" instead of DOM_EVENT.CLICK. I didn't find a simple solution for both enforcing this and re-using TS event maps. I think this is an acceptable compromise. We might want to remove the DOM_EVENT enum altogether, since it's no longer needed to enforce valid event names. Let's chat.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review March 31, 2023 15:21
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners March 31, 2023 15:21
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Contributor

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

LGTM

@BenoitZugmeyer BenoitZugmeyer merged commit d645ba2 into main Apr 4, 2023
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/improve-add-event-listener-typings branch April 4, 2023 10:16
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