React: Use specific event type for nativeEvent property - #15610
#15610React: Use specific event type for nativeEvent property#156103 commits merged intoDefinitelyTyped:masterDefinitelyTyped/DefinitelyTyped:masterfrom p-jackson:generic-native-eventp-jackson/DefinitelyTyped:generic-native-eventCopy head branch name to clipboard
Conversation
|
types/react/index.d.ts to authors (@pspeter3 @vsiao @johnnyreilly @bbenezech @pzavolinsky @digiguru AssureSign (account can't be detected) Microsoft (account can't be detected)). Could you review this PR? Checklist
|
|
My worries about breaking existing code were confirmed when my changes broke the build. A solution I've found is that in TypeScript 2.3 there should be a new feature for type parameter defaults (microsoft/TypeScript#13487), which means the existing interface can be extended. I've tried the latest 2.3 dev build and pushed changes to show how that will work. I expect to come back to this PR once 2.3 is released (which might be around 18 April given their new release cadence?) |
|
|
||
| interface ClipboardEvent<T> extends SyntheticEvent<T> { | ||
| interface ClipboardEvent<T> extends SyntheticEvent<T, NativeClipboardEvent> { | ||
| clipboardData: DataTransfer; |
There was a problem hiding this comment.
You don't have to use a type parameter to do this. Try this:
interface ClipboardEvent<T> extends SyntheticEvent<T> {
nativeEvent: NativeClipboardEvent;
}Allows event handlers to fully use the `ev.nativeEvent` property without casting. But it does make the `React.MouseEvent` type slightly more complicated.
TypeScript 2.3 should be adding type parameter defaults, which allows the `nativeEvent` property to be made generic but without breaking existing code.
Overriding nativeEvent in this way maintains backwards compatibility and doesn't depend on bleeding-edge TypeScript features.
02795d2 to
7b9d4ea
Compare
|
Thanks for the tip @vsiao! I was worried what would happen to users who try to update |
|
This cause typescript compile error antd-tools run compile
/Users/jiangkai/github/ant-design-mobile/node_modules/@types/react/index.d.ts(369,15): error TS2430: Interface 'TouchEvent<T>' incorrectly extends interface 'SyntheticEvent<T>'.
Types of property 'nativeEvent' are incompatible.
Type 'NativeTouchEvent' is not assignable to type 'Event'.
Property 'bubbles' is missing in type 'NativeTouchEvent'.
/Users/jiangkai/github/ant-design-mobile/node_modules/@types/react/index.d.ts(427,46): error TS2344: Type 'TouchEvent<T>' does not satisfy the constraint 'SyntheticEvent<any>'.
Types of property 'nativeEvent' are incompatible.
Type 'NativeTouchEvent' is not assignable to type 'Event'.
[11:30:34] TypeScript: 2 semantic errors
[11:30:34] TypeScript: emit succeeded (with errors) |
|
@paranoidjk did the breakage occur because of React Native, or did this happen in web code? Do you think this will effect other React Native users? I was hoping to get some "thumbs up" reviews before this was merged, hopefully from someone with React Native experience? |
|
@p-jackson I guess it's because react-native, since https://github.com/ant-design/ant-design is pure web react components got no error, but https://github.com/ant-design/ant-design-mobile both have react and react-native components, which compile gets error. |
|
@paranoidjk Can you get a good repro for this? Just installing |
|
I have a repro @Andy-MS |
|
I need steps that would allow me to reproduce it. Just installing |
|
I tried also on top of https://github.com/ant-design/ant-design/ and it didn't repro. Any tips how one might be able to debug the conflicting |
|
Interesting and possibly related, I'm now seeing: So perhaps the issue really is with react-native. But, in that case I would also like us to fix that ;-) |
|
Actually, the Map issue was fixed by changing |
|
So, the problem is it thinks |
|
Yep that did it! Thanks @Andy-MS @paranoidjk try upgrading your |
|
@Andy-MS with latest typescript et. al., now actually having this with react-native Would this be a problem with the React typings or any other pointers? |
|
@bherila That looks like the same error as before. Check the definition of |
|
This time, the solution was to upgrade Thanks @Andy-MS !! |
This change probably needs discussion.
This allows event handlers to fully use the
ev.nativeEventproperty without casting. But it does make theReact.MouseEventtype slightly more complicated and is a breaking change for users usingReact.SyntheticEventdirectly.The case that led me to opening the PR is: I have a mouse event I'd like to access the
movementXproperty on, but can see from React's synthetic mouse event docs that they don't supportmovementX, so I need to get it through thenativeEventproperty.Something I'd like feedback on is whether the way I'm getting access to the native event types from within the namespace is the best way. Is there something like
::GlobalTypein Typescript?Could be related to #6883, but I'm not thinking of the React Native case (I haven't used it before).
Please fill in this template.
Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Follow the advice from the readme.
Avoid common mistakes.
Run
npm run lint package-name(ortscif notslint.jsonis present).Provide a URL to documentation or source code which provides context for the suggested changes:
https://developer.mozilla.org/en-US/docs/Web/API/Event
https://facebook.github.io/react/docs/events.html
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a
tslint.jsoncontaining{ "extends": "../tslint.json" }.