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 bau.d.ts to have automatic types for event listeners as props #108

Closed
wants to merge 3 commits into from

Conversation

louwers
Copy link
Contributor

@louwers louwers commented Sep 13, 2024

Now

button({
  onclick: (e) => {}
});

will have the correct type for e (and not any).

I removed the other types in the PropsHTMLElement union since they are duplicated in Props.

@FredericHeem
Copy link
Member

Thanks for the PR, unfortunately, it creates a regression:

cd bau/examples/bau-ts-test
npm run build
src/main.ts:469:11 - error TS2345: Argument of type '{ placeholder: string; value: State<string>; oninput: ({ target }: {    target: HTMLInputElement;}) => string; }' is not assignable to parameter of type 'PropsAll<HTMLInputElement> | undefined'.
  Types of property 'oninput' are incompatible.
    Type '({ target }: {    target: HTMLInputElement;}) => string' is not assignable to type '(this: GlobalEventHandlers, ev: Event) => any'.
      Types of parameters '__0' and 'ev' are incompatible.
        Type 'Event' is not assignable to type '{ target: HTMLInputElement; }'.
          Types of property 'target' are incompatible.
            Type 'EventTarget | null' is not assignable to type 'HTMLInputElement'.
              Type 'null' is not assignable to type 'HTMLInputElement'.

469     input({
              ~
470       placeholder: "Enter username",
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... 
473         (inputState.val = target.value),
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
474     }),
    ~~~~~


@louwers
Copy link
Contributor Author

louwers commented Sep 13, 2024

Makes sense since it was typed as any before.

@FredericHeem
Copy link
Member

This change is breaking other's people code. Is there a way to avoid it ?

@FredericHeem
Copy link
Member

Can you review the PR #109 ?

@louwers
Copy link
Contributor Author

louwers commented Sep 13, 2024

It's not technically breaking their code, but their code may not typecheck after upgrading. But the point of this PR is to use correct types.

If you want to use semver for TypeScript types, the best solution would be to include this change in a future major release.

@FredericHeem
Copy link
Member

If someone updates to a new version and the build fails, it is a breaking change.

@louwers
Copy link
Contributor Author

louwers commented Sep 13, 2024

@FredericHeem
Copy link
Member

With this PR, on an input element, the oninput handler has the event parameter typed Event but should be InputEvent

Screenshot 2024-09-13 at 19 13 29

@louwers
Copy link
Contributor Author

louwers commented Sep 14, 2024

@FredericHeem No, TypeScript uses Event for these types of events because it is not always an InputEvent. See reasoning here: microsoft/TypeScript-DOM-lib-generator#1174 (comment)

@FredericHeem
Copy link
Member

In the case of input of type text, the event type is InputEvent but the typescript bindings still use Event. This is still on open issue in the IMHO. See comment microsoft/TypeScript-DOM-lib-generator#1174 (comment)
The following code should be valid:

input({
      type:"text"
      oninput: (event: InputEvent) => console.log(event.data),
    }),

@louwers
Copy link
Contributor Author

louwers commented Sep 14, 2024

There are no specific types in the TypeScript DOM lib for a HTMLInputElement that is of type="text".

You would have to use

input({
      type:"text"
      oninput: (event) => console.log((event as InputEvent).data),
    }),

You could probably hack your way around this but it would introduce very complicated types and it's not worth the effort in my opinion. It is better to wait until TypeScript catches up (if it will). There is also a chance the HTML standard will be modified whatwg/html#682

@FredericHeem
Copy link
Member

I have yet to see any benefit of this approach. Moreover that will break existing code.

@louwers
Copy link
Contributor Author

louwers commented Sep 15, 2024

Automatic types for Events. Especially when working with (type-safe_ JavaScript this can save some typing.

I can see if I can think of a backward-compatible approach.

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