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

[TS] <DraggableCore>'s event handlers are not using React events, but the typing said so #373

Closed
be5invis opened this issue Dec 21, 2018 · 11 comments

Comments

@be5invis
Copy link
Contributor

export type DraggableEventHandler = (e: React.MouseEvent<HTMLElement> | React.TouchEvent<HTMLElement>, data: DraggableData) => void | false;

should be

export type DraggableEventHandler = (e: MouseEvent | TouchEvent, data: DraggableData) => void | false;
@be5invis
Copy link
Contributor Author

@matoilic @STRML

@STRML
Copy link
Collaborator

STRML commented Dec 21, 2018

What's the actual error?

@be5invis
Copy link
Contributor Author

@be5invis
In my app, from the typing info I used e.presist() and grabbed the native event via e.nativeEvent and it failed like Uncaught TypeError: event.persist is not a function.
Your TS typing in 3.1 is misguiding people.

@STRML
Copy link
Collaborator

STRML commented Dec 21, 2018

It is a React.MouseEvent and persist is defined. What is the actual code you are running?

@be5invis
Copy link
Contributor Author

I am using DraggableCore. Maybe DraggableCore is not using React events?

Some code:

function handleDrag(e: React.MouseEvent<HTMLElement> | React.TouchEvent<HTMLElement>, d: DraggableData) {
	e.persist();
	somethingElse(e.nativeEvent);
}

<DraggableCore
	onDrag={handleDrag}
>
	<div>
		test
	</div>
</DraggableCore>

@STRML
Copy link
Collaborator

STRML commented Dec 21, 2018

Ah, I see now why this happens: https://github.com/mzabriskie/react-draggable/blob/master/lib/DraggableCore.js#L297

This is not a React-wrapped event. Brainstorming how to address it.

@be5invis
Copy link
Contributor Author

be5invis commented Dec 21, 2018

@STRML
But for things like DraggableCore, we may want native events for performance or fine tuning.
As for immediate fix, just add a new type CoreDraggableEventHandler and type parameters as native events.

@be5invis be5invis changed the title [TS][Regression] DraggableEventHandler' s first parameter is not React event [TS] <DraggableCore>'s event handlers are not using React events, but the typing said so Dec 21, 2018
@STRML
Copy link
Collaborator

STRML commented Dec 21, 2018

Since SyntheticEvent is pooled the performance impact will be negligible. We can't just adjust the typedefs - for instance, if the drag stop is initiated by a mouseup outside of the wrapped element, it will be a regular MouseEvent, not a React.MouseEvent. So where your mouse happens to be affects the callback's shape.

@STRML
Copy link
Collaborator

STRML commented Dec 21, 2018

I don't see any good way to arbitrarily wrap a native event in a SyntheticEvent, so my proposal is to change the typedef:

  export type DraggableEventHandler = (
    e: React.MouseEvent<HTMLElement> | React.TouchEvent<HTMLElement> | MouseEvent | TouchEvent,
    data: DraggableData
  ) => void | false;

Please try it out and open a PR if it works for you.

@be5invis
Copy link
Contributor Author

be5invis commented Dec 21, 2018

@STRML LGTM
Perhaps this is more accurate

export type DraggableEventHandler = (
    e: React.MouseEvent<HTMLElement | SVGElement> | React.TouchEvent<HTMLElement | SVGElement> | MouseEvent | TouchEvent,
    data: DraggableData
  ) => void | false;

@STRML
Copy link
Collaborator

STRML commented Apr 11, 2020

Fixed in #374

@STRML STRML closed this as completed Apr 11, 2020
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

No branches or pull requests

2 participants