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

TypeScript: Event properties can be omitted but not set to undefined #1640

Closed
WesSouza opened this issue Mar 22, 2023 · 5 comments · Fixed by ryansolid/dom-expressions#336
Closed
Labels
PRs Welcome An issue which might not be prioritized right away but would be open to community contribution typescript relating to typescript or types

Comments

@WesSouza
Copy link

WesSouza commented Mar 22, 2023

Describe the bug

The types for events are optional, but require a valid value if set.

Optional properties should allow undefined in case we want to programmatically set the property, such as in the example below, as the effect is most likely the same.

export function Test(props: {
  onClick?: () => void
}) {
  return (
    <>
      <button>No Errors</button>
      <button onClick={props.onClick}>Test</button>
      {/* ^ Type 'undefined' is not assignable to type 'EventHandlerUnion<HTMLDivElement, MouseEvent>'. ts(2375) */}
    </>
  );
}

Edit: adjusted example.

@otonashixav
Copy link
Contributor

In general Solid's types aren't compatible with exactOptionalPropertyTypes, though a PR adding undefined as appropriate would be welcome.

In the specific example provided however, setting onClick to undefined won't do anything since

  1. Events on DOM elements aren't reactive, so changing onClick wouldn't change the event handler from its initial value (do (event) => props.onClick?.(event) instead) and
  2. You're not reading onClick in a reactive scope by destructuring (unless you're using a plugin that reinserts the prop access).

@WesSouza
Copy link
Author

Thanks for the note on props, I still need to get properly familiar with the reactive-gotchas-for-React-developers 😅.

I see all of the properties are described in the jsx.d.ts file using EventHandlerUnion.

I wonder if it would be acceptable to just change that type to:

type EventHandlerUnion<T, E extends Event> = EventHandler<T, E> | BoundEventHandler<T, E> | undefined;

@otonashixav
Copy link
Contributor

The types are in dom-expressions.

@WesSouza
Copy link
Author

I can’t find anywhere that brings that codebase into the solid package. How does that work?

@ryansolid
Copy link
Member

It's because dom-expressions is sort of a framework builder. In order to get a treeshakeable output we need to basically bundle it into Solid's output.

@ryansolid ryansolid added the typescript relating to typescript or types label Mar 23, 2023
@ryansolid ryansolid added the PRs Welcome An issue which might not be prioritized right away but would be open to community contribution label May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PRs Welcome An issue which might not be prioritized right away but would be open to community contribution typescript relating to typescript or types
Projects
None yet
3 participants