Skip to content

[Web] v3 typing and using handler data #3651

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 75 commits into from
Aug 19, 2025
Merged

[Web] v3 typing and using handler data #3651

merged 75 commits into from
Aug 19, 2025

Conversation

akwasniewski
Copy link
Contributor

@akwasniewski akwasniewski commented Aug 1, 2025 •
edited
Loading

Description

Changing web implementation to use v3 types with a proper handlerData field.

Test plan

check whether handledData is a proper field of nativeEvent when using NativeDetector.

  const gesture = useGesture('PanGestureHandler', {
    onBegin: (e: any) => {
      console.log('onGestureHandlerEvent', e.nativeEvent.handlerData);
    },
  });

akwasniewski and others added 30 commits July 25, 2025 11:31
@akwasniewski
basic component
ea8bd73
@akwasniewski
Merge branch 'next' into @akwasniewski/native-detector-web
32fea07
@akwasniewski
better useffect triggers:
968112c
@akwasniewski
added todos
6a69cd7
@akwasniewski
moved Gesture handler detectors
2b404af
@akwasniewski
path fix
9c30c1a
@akwasniewski
extracted host gesture detector
d7dd057
@akwasniewski
not extending view props
56b93d3
@akwasniewski
native detector action type
ad8ef64
@akwasniewski
moved implementation of web handler
0304177
@akwasniewski
memoisation
4aaf700
@akwasniewski
detaching handlers
eeddd24
@akwasniewski
forgot to asign to oldHandlerTags
3242ffd
@akwasniewski
removed unnecessary types
7147ab3
@akwasniewski
native detector animated event action type
cc78f74
@akwasniewski
removed console log
067becb
@akwasniewski
handling animated events in sendEvents
d782c95
@akwasniewski
first refactor
86046bd
@akwasniewski
using native detector animated event
6c34d33
@akwasniewski
refactor 2
cd793b3
@akwasniewski
use callback in native detector
580494d
@akwasniewski
tagMessage
eb98bf4
@akwasniewski
should prevent drop
af663c8
@akwasniewski
old api compatibility
42e84b4
@akwasniewski
not throwing strings
079fdb4
@akwasniewski
removed unnessary check
cb72d7e
@akwasniewski
even more refactoring
de98017
@akwasniewski
throwing when propsref is null
e193ff4
@akwasniewski
helper action type functions
2ba053a
@akwasniewski
separate paths on touch events
8ab666b
@akwasniewski akwasniewski marked this pull request as draft August 7, 2025 06:25
@akwasniewski akwasniewski force-pushed the @akwasniewski/v3-web-typing branch from 91aad1d to 5837159 Compare August 8, 2025 06:18
Base automatically changed from @akwasniewski/native-detector-web to next August 8, 2025 08:57
@akwasniewski akwasniewski marked this pull request as ready for review August 12, 2025 09:53
@akwasniewski akwasniewski requested a review from m-bert August 12, 2025 10:38
m-bert
m-bert reviewed Aug 14, 2025
View reviewed changes
packages/react-native-gesture-handler/src/web/interfaces.ts Outdated
Comment on lines 113 to 116
type ResultEventType = GestureHandlerEvent<unknown> | GestureHandlerNativeEvent;
export interface ResultEvent<T extends ResultEventType = ResultEventType>
extends Record<string, T | number> {
nativeEvent: T;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but GestureHandlerEvent already has nativeEvent field (look here for context). In that case, we will have event type where it is possible to have nativeEvent.nativeEvent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I find it quite confusing with this default value, wouldn't it be easier to read if we had just ResultEvent<T> and then pass either GestureHandlerEvent, GestureHandlerNativeEvent or TouchEvent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but GestureHandlerEvent already has nativeEvent

No, types in GestureHandlerEvent are wrappers on payload types defined here, and only add handlerData. Those types do not have nativeEvent field

Also I find it quite confusing with this default value

Ok, it was inspired by this. I thought that it would simplify the code as we would not have to write <unknown> everywhere. I don't have a strong preference, if you think it's better to add <unknown> i can change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you think it's better to add i can change it.

What do you think I should do? I feel that current solution is stronger typed, what I think is better

packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts Outdated
@@ -369,7 +370,7 @@ export default abstract class GestureHandler implements IGestureHandler {
const { onGestureHandlerEvent, onGestureHandlerTouchEvent }: PropsRef =
this.propsRef!.current;

const touchEvent: ResultTouchEvent | undefined =
const touchEvent: ResultEvent<TouchEvent> | undefined =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that TouchEvent will have correct fields and won't expose something that we do not provide?

Copy link
Contributor Author

@akwasniewski akwasniewski Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, gestureTouchEvent defined here, on which TouchEvent is a union has the same fields as the old nativeTouchEvent defined within web/interfaces.

Copy link
Contributor Author

@akwasniewski akwasniewski Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now when I think about it we could probably do the same for GestureHandlerNativeEvent, is there a reason why v2 redeclared those types for the web, when we already have similar types in commons?
EDIT: ok, nevermind they have conflicting type

m-bert
m-bert approved these changes Aug 19, 2025
View reviewed changes
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akwasniewski akwasniewski merged commit 8b0f128 into next Aug 19, 2025
1 check passed
@akwasniewski akwasniewski deleted the @akwasniewski/v3-web-typing branch August 19, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-bert m-bert m-bert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akwasniewski @m-bert