-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[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.
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
base: next
Are you sure you want to change the base?
Conversation
@@ -406,7 +405,6 @@ export default abstract class GestureHandler implements IGestureHandler { | |||
invokeNullableMethod(onGestureHandlerStateChange, resultEvent); | |||
} | |||
if (this.state === State.ACTIVE) { | |||
resultEvent.nativeEvent.oldState = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I remember this was necessary in order to keep compatibility with old API. Right, @j-piasecki?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the case referred in the comment above as
this may cause trouble in the future
Due to how new events are structures (no oldState
field in update) we could either do a weird conditionality check as in 0c9692 or change the new types so they all include 'oldState'. Both solutions seem off, do you maybe have any better ideas?
oldState: oldState, | ||
handlerData: { | ||
pointerType: this.pointerType, | ||
numberOfPointers: this.tracker.trackedPointersCount, | ||
pointerInside: this.delegate.isPointerInBounds( | ||
this.tracker.getAbsoluteCoordsAverage() | ||
), | ||
...this.transformNativeEvent(), | ||
target: this.viewRef, | ||
}, | ||
} as StateChangeEvent<unknown> | UpdateEvent<unknown>, |
There was a problem hiding this comment.
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 this will break compatibility with V2 as all events will have handlerData
field, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, should be fixed in 04ec35f
91aad1d
to
5837159
Compare
} as StateChangeEvent<unknown> | UpdateEvent<unknown>, | ||
timeStamp: Date.now(), | ||
}; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can either use ternary operator for conditional return ?:
, or remove this else
statement as it doesn't do anything 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true hah, fixed in 0c9692
timeStamp: number; | ||
} | ||
|
||
// We need to leave any for v2 compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to fix the underlying issue, thus the comment became obsolete. Removed in 2e47b1ae.
get initialized(): boolean { | ||
return this.isInitialized; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't it added in other PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it was no longer necessary so it was removed. I didn't notice it during manual merge so it remained here. Fixed 1cd5eef
type ResultEventType = GestureHandlerEvent<unknown> | GestureHandlerNativeEvent; | ||
export interface ResultEvent<T extends ResultEventType = ResultEventType> | ||
extends Record<string, T | number> { | ||
nativeEvent: T; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
private ensureViewRef(viewRef: any): asserts viewRef is number { | ||
if (!viewRef) { | ||
throw new Error(tagMessage('Cannot handle event when target is null')); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, little deja vu - wasn't it added in other PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar function, ensurePropsRef
was added here
@@ -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 = |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Description
Changing web implementation to use v3 types with a proper
handlerData
field.Test plan
check whether
handledData
is a proper field ofnativeEvent
when usingNativeDetector
.