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

Update "Typing Events" section to discuss TargetedEvent and subtypes #1098

Open
tjcrowder opened this issue Mar 22, 2024 · 9 comments
Open

Comments

@tjcrowder
Copy link

tjcrowder commented Mar 22, 2024

Currently the typing events section of the TypeScript documentation just mentions DOM event types and using a this type annotation to control the type of the element associated with the event (which gives a useful type to this but not to event.currentTarget). But now, TargetedEvent and its subtypes are used in the definitions of event types on HTML elements and provide a way to type currentTarget via a type argument. I think the section should be updated and expanded. I'd be happy to do a PR for it. (I may need some guidance, I haven't done any Preact docs stuff before.)

I'm thinking the update would:

  1. Describe TargetedEvent and its subtypes.
  2. Say that you can continue to use DOM event types if you like.
  3. Show examples of function components with event handlers not defined inline (and thus not having an inferred type for event).
  4. Mention that although TargetedEvent is on JSX currently, that may change (if it's still true that it may change?).
  5. Explain why TargetedEvent is re-exported by preact/compat (but its subtypes aren't). (?)
  6. Describe reusing HTMLAttributes on your own components that wrap DOM elements.

Would a PR along those lines be useful? If so, should it cover anything else?

@rschristian
Copy link
Member

rschristian commented Mar 22, 2024

Absolutely, any time/writing you'd be willing to donate would be massively appreciated!

The docs are fairly simple, every page is a markdown file living in content/*. The TS doc specifically lives here. I need to update the contributing doc (sorry!), but it's just a matter of running npm install && npm run dev, with the docs site opening at localhost:8080. You can edit the markdown and it will be reflected on the page (might need a refresh, we have a Vite migration underway and I can't quite remember if Webpack reloads or not).

I think it's fine to say TargetedEvent is on JSX without any mention of that changing, as the PR seems to have been stalled. I'm not familiar with some of the intricacies there, couldn't say whether it'll ever land. We certainly try to avoid any breaking changes, so these types will live on JSX at least for the lifespan of 10.x

Explain why TargetedEvent is re-exported by preact/compat (but its subtypes aren't). (?)

Types are generally added to compat when someone asks for them. If something's missing that's needed, feel free to add. Should just be a matter of re-exporting here. I don't think there's any intent on not offering anything.

Otherwise, this sounds brilliant! Anything you add would be super appreciated, we obviously haven't touched the TS doc in a while.

@tjcrowder
Copy link
Author

Thanks for the orientation, @rschristian! Looks straightforward.

Thanks also for your comment on SO, sorry for my misunderstanding. Is there someone on the team we could check with about this? I want to be sure to provide the right guidance in the docs. It seems to me that using TargetedEvent on the types of the onXyz properties on the HTML elements strongly suggests that that's the current thinking for how to type handler parameters (though you can of course use the DOM types as well), but if I'm wrong or possibly wrong there I'd like to get it straight so I can document it correctly.

@rschristian
Copy link
Member

No worries of course, I just didn't want to lead you down the wrong path is all.

@marvinhagemeister and/or @JoviDeCroock might be more familiar, if they have the time (context, preactjs/preact#4309).

@tjcrowder
Copy link
Author

@marvinhagemeister @JoviDeCroock I'm still keen to contribute an update to this section, but I don't know what to write, it really depends on what the Preact team's position is on typing event handlers. jsx.d.ts uses TargetedEvent for EventHandler so that's what's inferred for the onXyz props, but when I went to run with that @rschristian said he wasn't sure they're "the current way" (in a comment on a now-deleted answer to this SO question). What would you like to see documented for this?

  1. Use TargetedEvent and its relatives?
  2. Use DOM event types?
  3. Use either of the above, it's a matter of style?
  4. Something else?

TIA

@JoviDeCroock
Copy link
Member

I personally point folks at these types https://github.com/preactjs/preact/blob/main/src/jsx.d.ts line 1496 MouseEventHandler for instance. When defining the event handler outside of JSX these helpers should enable you

@tjcrowder
Copy link
Author

@JoviDeCroock Thanks! Those use TargetedEvent, so are you saying Option 1? Or Option 4 (use the XYZHandler types and avoid using TargetedEvent and its relatives directly)? Or...

I usually type the event handler's parameter rather than the handler as a whole (not least so I can use a function declaration), so the XYZHandler types aren't what I'd naturally reach for. But if that's the preferred approach...

@JoviDeCroock
Copy link
Member

What I'm saying is that from my point of view both work, I personally am a big fan of using the Handler approach but I can also see why folks prefer using TargetedEvent directly so I would document both.

@tjcrowder
Copy link
Author

@JoviDeCroock Thanks, sounds good!

Here's my revised list of what I propose to cover:

  1. Show an inline handler, no explicit typing needed.
  2. Show handlers declared separately...
    • ...using MouseEventHandler and such
    • ...using TargetedEvent and such
    • ...using DOM event types
  3. Show reusing HTMLAttributes on your own components that wrap DOM elements.
  4. Remove references to FunctionComponent (context).

If folks are happy with that, I'll get going on it in the next couple of weeks.

@rschristian
Copy link
Member

Sounds great to me! Thanks again for looking into this.

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

No branches or pull requests

3 participants