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

Make React.SyntheticEvent.target generic over T, not React.SyntheticEvent.currentTarget #11508

Closed
Emm opened this Issue Sep 26, 2016 · 7 comments

Comments

Projects
None yet
7 participants
@Emm
Contributor

Emm commented Sep 26, 2016

Currently, the Typescript 2.0 signature for React's SyntheticEvent looks like:

interface SyntheticEvent<T> {
    currentTarget: EventTarget & T;
    target: EventTarget;
}

According to commit a13fa7a, target was originally EventTarget & T, but this change was (apparently) accidentally reverted during a merge in commit 5607f54.

The current signature breaks a lot of pre-2.0 code (as you can imagine, React code is replete with event handlers) for little to no benefit (since most code relies on event.target, so this means still relying on pre-2.0 code like (event.target as any).value, since event.target is not generic.

I'll be happy to provide a patch, but I'd like to be sure SyntheticEvent.target was not de-generified on purpose.

@andy-ms

This comment has been minimized.

Show comment
Hide comment
@andy-ms

andy-ms Sep 26, 2016

Contributor

This is probably an mistake made during merging. Please make a PR.

Contributor

andy-ms commented Sep 26, 2016

This is probably an mistake made during merging. Please make a PR.

@rohitlodha

This comment has been minimized.

Show comment
Hide comment
@rohitlodha

rohitlodha Sep 26, 2016

see #11041, #10784 and many more. Is a mystery why it is not fixed even after merge.

rohitlodha commented Sep 26, 2016

see #11041, #10784 and many more. Is a mystery why it is not fixed even after merge.

@bbenezech

This comment has been minimized.

Show comment
Hide comment
@bbenezech

bbenezech Oct 25, 2016

Contributor

You cannot always tell target's type at compile time. Making it generic is of little value.
target is the origin of the event (which no one really cares about, it might be a span inside a link, for example)
currentTarget is the element that has the event handler attached to, which you should very much care about and type accordingly if you attached a dataset or other attributes to it, and intend to access at runtime.

Relying on target instead of currentTarget is a beginner's mistake that will bite them sooner than latter.

For example:

  handleTabClick = (e: React.SyntheticEvent<HTMLLinkElement>) => {
    e.preventDefault()
    // !!! e.target.dataset is empty if user clicked the <span>, not the <a> !!!
    // !!! how can you type e.target? User might also have clicked the <a> if it has padding !!!
    // you have to use currentTarget, and it is the element that you know the type for sure
    if (e.currentTarget.dataset['closable'] === 'true') {
      this.close()
    }
  }

render() {
  return <a onClick={this.handleTabClick} data-closable="true">
      <span className="fatty">Click me!</span>
   </a>
}

This code used to compile, as it should.

With the 'fix' merged, e.currentTarget is not typable (I can't access its dataset without a type error) and e.target is now typed as an HTMLLinkElement, when in fact there is no way we can be sure about it. (I may have clicked the span inside.

Theoretically we could add a second generic to SyntheticEvent, to type target, when user is certain about it (no children to the event's handler, or they all have the same type). But it is of little value and really misleading (because it only applies to particular cases where using casting would make a lot more sense).
In particular, if you have no children, you can use currentTarget.

Contributor

bbenezech commented Oct 25, 2016

You cannot always tell target's type at compile time. Making it generic is of little value.
target is the origin of the event (which no one really cares about, it might be a span inside a link, for example)
currentTarget is the element that has the event handler attached to, which you should very much care about and type accordingly if you attached a dataset or other attributes to it, and intend to access at runtime.

Relying on target instead of currentTarget is a beginner's mistake that will bite them sooner than latter.

For example:

  handleTabClick = (e: React.SyntheticEvent<HTMLLinkElement>) => {
    e.preventDefault()
    // !!! e.target.dataset is empty if user clicked the <span>, not the <a> !!!
    // !!! how can you type e.target? User might also have clicked the <a> if it has padding !!!
    // you have to use currentTarget, and it is the element that you know the type for sure
    if (e.currentTarget.dataset['closable'] === 'true') {
      this.close()
    }
  }

render() {
  return <a onClick={this.handleTabClick} data-closable="true">
      <span className="fatty">Click me!</span>
   </a>
}

This code used to compile, as it should.

With the 'fix' merged, e.currentTarget is not typable (I can't access its dataset without a type error) and e.target is now typed as an HTMLLinkElement, when in fact there is no way we can be sure about it. (I may have clicked the span inside.

Theoretically we could add a second generic to SyntheticEvent, to type target, when user is certain about it (no children to the event's handler, or they all have the same type). But it is of little value and really misleading (because it only applies to particular cases where using casting would make a lot more sense).
In particular, if you have no children, you can use currentTarget.

@bbenezech

This comment has been minimized.

Show comment
Hide comment
@bbenezech

bbenezech Oct 25, 2016

Contributor

Plus it is completely wrong, on a DOMAttributes<SomeType>, you get a onClick: MouseEventHandler<SomeType>, then get EventHandler<MouseEvent<SomeType>> with

    interface EventHandler<E extends SyntheticEvent<any>> {
        (event: E): void;
    }

You absolutely cannot type target differently from currentTarget. onClick expects an event handler that has handling event (aka currentTarget) as the generic. It cannot be something else, it won't compile.

Contributor

bbenezech commented Oct 25, 2016

Plus it is completely wrong, on a DOMAttributes<SomeType>, you get a onClick: MouseEventHandler<SomeType>, then get EventHandler<MouseEvent<SomeType>> with

    interface EventHandler<E extends SyntheticEvent<any>> {
        (event: E): void;
    }

You absolutely cannot type target differently from currentTarget. onClick expects an event handler that has handling event (aka currentTarget) as the generic. It cannot be something else, it won't compile.

@tkrotoff

This comment has been minimized.

Show comment
Hide comment
@tkrotoff

tkrotoff Oct 26, 2016

Contributor

@bbenezech

handleTabClick = (e: React.SyntheticEvent<HTMLLinkElement>) => { ... }

I suggest instead:

handleTabClick = (e: React.FormEvent<HTMLLinkElement>) => { ... }

You also have ClipboardEvent<T>, CompositionEvent<T>, DragEvent<T>, FocusEvent<T>, KeyboardEvent<T>...
SyntheticEvent is the low level thing.

Contributor

tkrotoff commented Oct 26, 2016

@bbenezech

handleTabClick = (e: React.SyntheticEvent<HTMLLinkElement>) => { ... }

I suggest instead:

handleTabClick = (e: React.FormEvent<HTMLLinkElement>) => { ... }

You also have ClipboardEvent<T>, CompositionEvent<T>, DragEvent<T>, FocusEvent<T>, KeyboardEvent<T>...
SyntheticEvent is the low level thing.

@matthew-rister

This comment has been minimized.

Show comment
Hide comment
@matthew-rister

matthew-rister Dec 16, 2016

This is still like this on the @types NPM repository. Any update on when this will be published

matthew-rister commented Dec 16, 2016

This is still like this on the @types NPM repository. Any update on when this will be published

@andy-ms

This comment has been minimized.

Show comment
Hide comment
@andy-ms

andy-ms Dec 16, 2016

Contributor

This was changed back. See #12239.

Contributor

andy-ms commented Dec 16, 2016

This was changed back. See #12239.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment