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

refactor: screen rewritten as functional component #2111

Merged
merged 3 commits into from
May 6, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 27 additions & 33 deletions src/components/Screen.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/no-var-requires */
import React from 'react';
import { Animated, View, Platform } from 'react-native';

Expand All @@ -17,7 +16,7 @@ import ScreenNativeComponent from '../fabric/ScreenNativeComponent';
import ModalScreenNativeComponent from '../fabric/ModalScreenNativeComponent';

export const NativeScreen: React.ComponentType<ScreenProps> =
ScreenNativeComponent as any;
ScreenNativeComponent as React.ComponentType<ScreenProps>;
const AnimatedNativeScreen = Animated.createAnimatedComponent(NativeScreen);
const AnimatedNativeModalScreen = Animated.createAnimatedComponent(
ModalScreenNativeComponent as React.ComponentType<ScreenProps>
Expand All @@ -42,27 +41,25 @@ interface ViewConfig extends View {
};
}

export class InnerScreen extends React.Component<ScreenProps> {
private ref: React.ElementRef<typeof View> | null = null;
private closing = new Animated.Value(0);
private progress = new Animated.Value(0);
private goingForward = new Animated.Value(0);
export const InnerScreen = React.forwardRef<View, ScreenProps>(
function InnerScreen(props, ref) {
const innerRef = React.useRef<ViewConfig | null>(null);
React.useImperativeHandle(ref, () => innerRef.current!, []);

Copy link
Member

Choose a reason for hiding this comment

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

What is this line for?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is intended to replace removed code mentioned above:
setNativeProps(props: ScreenProps): void { this.ref?.setNativeProps(props); }
However I am not sure wether it is correctly implemented

setNativeProps(props: ScreenProps): void {
this.ref?.setNativeProps(props);
}
const setRef = (ref: ViewConfig) => {
Comment on lines -51 to -53
Copy link
Member

Choose a reason for hiding this comment

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

Before we can remove this method as unused, we need to make sure no one actually uses it 😅 We need to:

  1. check react-navigation code for any usages of these,
  2. as this seems like method that could be utilised by react-native-reanimated - we need to also look for usages there (also see our ReanimatedScreenProvider) - it would be best to consult with reanimated team,
  3. thinking now, I would rather leave this method exposed in this PR 😄

Copy link
Member

Choose a reason for hiding this comment

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

Or is this intentional & you are utilising this method defined on innerRef?

Copy link
Member

Choose a reason for hiding this comment

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

I believe Animated still uses setNativeProps under the hood so I'd leave it. After quick talk with @piaskowyk, react-native-reanimated is not utilizing it, but still we would want it for Animated. @alduzy did you maybe check examples with Animated to see if they work when this code is removed? Would be also good to check with both useNativeDriver set to false and true and in TestsExample and FabricTestsExample.

innerRef.current = ref;
props.onComponentRef?.(ref);
};

setRef = (ref: React.ElementRef<typeof View> | null): void => {
this.ref = ref;
this.props.onComponentRef?.(ref);
};
Copy link
Member

@kkafar kkafar Apr 26, 2024

Choose a reason for hiding this comment

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

I would like to see some comment why do we resign from calling this callback here (onComponentRef)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an oversight, brought it back

const closing = new Animated.Value(0);
const progress = new Animated.Value(0);
const goingForward = new Animated.Value(0);

Copy link
Member

Choose a reason for hiding this comment

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

These are moved from state to being recreated on every rerender. Is that intentional? Do we expect any issues here? @WoLewicki asking for some insights here. I'm not really familiar with semantics of Animated values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe leveraging the useRef hook to store the values would be a good idea? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That was also my initial thought & I think we should go for this.

In any case I would like to hear from someone who might now some internals (already tagged).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I can see that in the example they use useRef for it: https://reactnative.dev/docs/animated#example. If it works correctly then it seems like the way to go. I can also see that a similar concept is used in react-navigation: https://github.com/react-navigation/react-navigation/blob/d0abdee67f5db8cf39112af535846ffededfb21d/packages/react-native-tab-view/src/useAnimatedValue.tsx#L4

render() {
const {
enabled = screensEnabled(),
freezeOnBlur = freezeEnabled(),
...rest
} = this.props;
} = props;

// To maintain default behavior of formSheet stack presentation style and to have reasonable
// defaults for new medium-detent iOS API we need to set defaults here
Expand Down Expand Up @@ -112,13 +109,13 @@ export class InnerScreen extends React.Component<ScreenProps> {
...ref.viewConfig.validAttributes.style,
display: false,
};
this.setRef(ref);
setRef(ref);
} else if (ref?._viewConfig?.validAttributes?.style) {
ref._viewConfig.validAttributes.style = {
...ref._viewConfig.validAttributes.style,
display: false,
};
this.setRef(ref);
setRef(ref);
}
};

Expand Down Expand Up @@ -148,9 +145,9 @@ export class InnerScreen extends React.Component<ScreenProps> {
[
{
nativeEvent: {
progress: this.progress,
closing: this.closing,
goingForward: this.goingForward,
progress,
closing,
goingForward,
},
},
],
Expand All @@ -168,9 +165,9 @@ export class InnerScreen extends React.Component<ScreenProps> {
) : (
<TransitionProgressContext.Provider
value={{
progress: this.progress,
closing: this.closing,
goingForward: this.goingForward,
progress,
closing,
goingForward,
}}>
{children}
</TransitionProgressContext.Provider>
Expand All @@ -195,25 +192,22 @@ export class InnerScreen extends React.Component<ScreenProps> {
return (
<Animated.View
style={[style, { display: activityState !== 0 ? 'flex' : 'none' }]}
ref={this.setRef}
ref={setRef}
{...props}
/>
);
}
}
}
);

// context to be used when the user wants to use enhanced implementation
// e.g. to use `useReanimatedTransitionProgress` (see `reanimated` folder in repo)
export const ScreenContext = React.createContext(InnerScreen);

class Screen extends React.Component<ScreenProps> {
static contextType = ScreenContext;
const Screen: React.FC<ScreenProps> = props => {
const ScreenWrapper = React.useContext(ScreenContext) || InnerScreen;

render() {
const ScreenWrapper = (this.context || InnerScreen) as React.ElementType;
return <ScreenWrapper {...this.props} />;
}
}
return <ScreenWrapper {...props} />;
};

export default Screen;
Loading