Revert "Make SyntheticEvent.target generic, not SyntheticEvent.currentTarget."

[bbenezech]

Reverts #11531

[bbenezech]

#11508 (comment) for context.

Do not revert back without notifying me before.

[vvakame]

sorry, @andy-ms could you check this PR?

[ghost]

Pinging @Emm, @Rohitlodha, @s-panferov, @nantaphop, @huan086, @alitaheri

[huan086]

I'll just reiterate my issue that started it all #10811

So merge this ASAP

e.currentTarget should be generic. I've no idea why so many people use e.target (including me in the past), introducing stupid bugs where I try to get attributes from the wrong element

[bbenezech]

@huan086 😍

[bbenezech]

@vvakame please merge ASAP, this is not a good idea to leave react.d.ts in this state. It has caused enough bugs and confusion as it is.

[tkrotoff]

@huan086 @bbenezech
All React documentation and ecosystem uses e.target.value, filling an issue there would be a great idea. I suggest an online React demo (Plunker, jsfiddle...) to show that it is dangerous.

There is a high probability that somebody else will change back to target in the future.
I would suggest a note in the d.ts file above target: EventTarget about this.

[huan086]

@tkrotoff I glanced through both links and do see that e.currentTarget is used. e.target is only used in the special case of input onChange, which is guaranteed to come from the input. So no issue there.

I like the idea of adding a note to remind people

[dsifford]

Thank you so much for this @bbenezech 👍

[kengorab]

Can this be the last time this changes, or at least not have them be patch-level changes? We've had our build broken twice now because of this.

[ghost]

@bbenezech Could you make a PR adding a comment telling people not to change it back?

[Strato]

We had our build broken twice as well. Make up your mind and please stop this mess.

And when you do breaking changes, please bump version according to semver semantics.

[bbenezech]

@Strato @kengorab How do you define Semver in the context of a typescript definition file? Specifically, what is its public API?
Or with a reduction proof attempt: what change can be made without potentially breaking a build?

AFAIK, versioning is automatic with changes on branch types-2.0: each commit bumps the patch bit. Major and minor bits are from the underlying lib.

I may be wrong, but to me it looks like you have non-deterministic npm builds and whine on maintainers when it blows at your face.
If that is the issue, do not do that. Instead use yarn lock, shrinkwrap or lock the last digit in package.json. Even semver libs should be expected to have regressions, and this ought not break your testing or production build.

[kengorab]

each commit bumps the patch bit

I wasn't aware of that, sorry.

what change can be made without potentially breaking a build?

Adding a function usually qualifies as a non-breaking change, so in the context of a type definition file, I'd equate that to either adding a definition for a function that was previously left out of the file, or refining a type to something more specific (say, changing an instance of any to something more useful). Either of those could potentially break someone's build, I suppose, if they were incorrectly using a function or a type. But I think the onus would be on the user in that case, since they were implicitly relying on the incompleteness of the previous definition file.

I don't think adding completeness should count as a breaking change, but altering a type should, in my opinion. If I had a function foo(bar: number | string): number and its signature was changed to foo(bar: number): number, that would definitely be considered a breaking change, as far as an API would be concerned. I don't expect the versioning scheme to change for this repo though; keeping the major and minor in sync with the main lib is good practice.

non-deterministic npm builds

I do use yarn locally, but our build boxes still use npm. The change to yarn there is outside of my control. I'll definitely be more conscientious of locking down version numbers in the future for type definition files, though.

[skeate]

Not to beat this dead horse any more, but with conditional types coming out in 2.8, could we add the & T back iff T is an empty element (e.g. HTMLInputElement)?

[karol-majewski]

@skeate Do you mean like this?

/**
 * @see https://developer.mozilla.org/en-US/docs/Glossary/Empty_element
 */
type EmptyHTMLElement =
    | HTMLAreaElement
    | HTMLBaseElement
    | HTMLBRElement
    | HTMLTableColElement
    | HTMLEmbedElement
    | HTMLHRElement
    | HTMLImageElement
    | HTMLInputElement
    | HTMLLinkElement
    | HTMLMetaElement
    | HTMLParamElement
    | HTMLSourceElement
    | HTMLTrackElement;

  interface SyntheticEvent<T = Element> {
    /* ... */
    target: T extends EmptyHTMLElement
      ? EventTarget & T
      : EventTarget;
  }

We could even go one step futher and replace EventTarget & T with just T, since every T extends the EventTarget interface anyway.

This would make React typings (and their dependants) work only with TypeScript ^2.8, though.

[skeate]

Yep, that's what I was thinking. I'm not sure if/how this system handles different typescript versions (I see this note but I'm not sure what the implications of that are).