-
-
Notifications
You must be signed in to change notification settings - Fork 518
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
refactor: separate Screen native props and public API #2423
refactor: separate Screen native props and public API #2423
Conversation
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 havent read carefully, just skimmed through the changes. Leaving my initial impressions below
@@ -13,6 +13,10 @@ import type { | |||
// eslint-disable-next-line @typescript-eslint/ban-types | |||
type ScreenEvent = Readonly<{}>; | |||
|
|||
type ScreenTargetEvent = Readonly<{ |
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.
What is the reasoning behind this change here?
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.
This is how our public API looks:
onAppear?: (e: NativeSyntheticEvent<TargetedEvent>) => void;
Thus, the change. I needed to write in on my won cause TargetedEvent is {target: number}
, and Codegen doesn't like number
:D
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.
Changed in: cd5ccea. We decided not to change it because updating NativeProps will produce unused properties in C++ - unnecessarily wasting memory. My understanding is that the target value in an event is filled by the ReactNative thus mismatch between NativeProps and React props.
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.
@kkafar If I missed something would be good to put it here, as I linked the discussion in comment.
src/types.tsx
Outdated
onGestureCancel?: ( | ||
e: NativeSyntheticEvent<Readonly<Record<string, never>>>, | ||
) => void; |
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.
Why do we change this?
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.
To conform with NativeProps, I don't think this is breaking for anyone as we don't pass anything in this callback. I just described nothing differently.
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.
Do we stil need this change? I expect this to not change in effect of splitting out props of native components.
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.
Yeah, nothing changed here
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.
Reverted in: c51f636
## Description Sync ScreenNative and ModalScreenNative props.
…fter syncing props
…ce-from-the-public-API
…-public-API' of github.com:software-mansion/react-native-screens into @maciekstosio/Separate-native-Screen-interface-from-the-public-API
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.
Left few remarks.
src/components/Screen.tsx
Outdated
import NativeScreen from '../fabric/ScreenNativeComponent'; | ||
import ModalScreenNative from '../fabric/ModalScreenNativeComponent'; |
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.
Let's keep one convention. I believe we use xxxNativeComponent
everywhere.
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.
src/types.tsx
Outdated
onGestureCancel?: ( | ||
e: NativeSyntheticEvent<Readonly<Record<string, never>>>, | ||
) => void; |
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.
Do we stil need this change? I expect this to not change in effect of splitting out props of native components.
src/components/Screen.tsx
Outdated
type NativeScreenOverrideProps = Omit< | ||
React.ComponentPropsWithRef<typeof NativeScreen>, | ||
'onAppear' | 'onDisappear' | 'onWillAppear' | 'onWillDisappear' | ||
> & | ||
SharedOverride; | ||
|
||
type NativeModalScreenOverrideProps = Omit< | ||
React.ComponentPropsWithRef<typeof ModalScreenNative>, | ||
'onAppear' | 'onDisappear' | 'onWillAppear' | 'onWillDisappear' | ||
> & | ||
SharedOverride; |
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.
Single type should be enough. ModalScreen & Screen should use the same types I believe.
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 didn't want to combine them as they are different entities we try to sync them, but when casting, I think using just one could lead to undetected bugs.
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.
We could go with something like this:
type SharedOverride = {
onAppear?: ScreenProps['onAppear'];
onDisappear?: ScreenProps['onDisappear'];
onWillAppear?: ScreenProps['onWillAppear'];
onWillDisappear?: ScreenProps['onWillDisappear'];
}
type NativeScreenOverrideProps = Omit<
React.ComponentPropsWithRef<typeof ScreenNativeComponent | typeof ModalScreenComponent>,
"onAppear" | "onDisappear" | "onWillAppear" | "onWillDisappear"
> & SharedOverride;
const AnimatedNativeScreen = Animated.createAnimatedComponent(ScreenNativeComponent as React.ComponentType<NativeScreenOverrideProps>);
const AnimatedNativeModalScreen = Animated.createAnimatedComponent(ModalScreenComponent as React.ComponentType<NativeScreenOverrideProps>);
It should actually help us with some of out of sync problems and solves both issues
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.
There should be no differences in types between these components. At least for now - so single type is enough.
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.
src/components/Screen.tsx
Outdated
type NativeScreenOverrideProps = Omit< | ||
React.ComponentPropsWithRef<typeof NativeScreen>, | ||
'onAppear' | 'onDisappear' | 'onWillAppear' | 'onWillDisappear' | ||
> & | ||
SharedOverride; |
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, can we use Omit
somehow directly with SharedOverride
, so that there is single source of truth? keyof
or something?
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.
Just as I've done here
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.
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.
Okay, I think we're good. You can try to solve the typescript issue with empty object for onGestureCancel
callback in separate PR 🙏🏻
## Description Continuation of #2423. Separate public `ScreenStackProps` and `NativeProps` by not casting NativeComponent as `React.ComponentType<ScreenStackProps> `. ## Checklist - [ ] Ensured that CI passes
Description
Separating native types and public API for Screen component. The core idea behind this change is that I removed hardcoded as ScreenProps and try to conform public API to NativeProps accepted by Codegen, with some transformations if necessary.
Changes
The biggest change is not casting Native to Component with ScreenProps.
Screenshots / GIFs
N/A
Checklist