-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] [TS migration] Migrate 'withNavigationFallback.js' HOC to TypeScript #30072
[No QA] [TS migration] Migrate 'withNavigationFallback.js' HOC to TypeScript #30072
Conversation
ref={ref} | ||
/> | ||
) : ( | ||
<NavigationContext.Provider value={navigationContextValue as unknown as NavigationProp<ParamListBase>}> |
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 need this assertion?
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.
@fabioh8010 Because NavigationContext.Provider
value is expected to be of type NavigationProp<ParamListBase>
, but to make our navigationContextValue be of this type, we need to add a lot of methods like navigate, reset, goBack, etc. If you have any ideas on how to improve it, please let me know.
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'm trying to understand why do we pass only this values in the first place 🤔. It seems that only Button
component uses this HOC, could you check what props from this HOC are really being injected in the component?
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.
@fabioh8010 Let me provide you with a little bit more context.
This HOC is a workaround for cases when Button/Composer is used outside of the Navigation container. isFocused
param is used in button, addListener
and removeListener
are used in Composer.
It's the second PR for this file to migrate, that was the first.
In the previous one, we had a suggestion that this HOC isn't necessary anymore and removed it. It caused regressions.
This time I investigated it more careful, and it's not possible to get rid of this HOC, because ErrorBoundary is located outside of the NavigationContainer an has a fallback component GenericErrorPage which has Buttons inside. So without this HOC usage it'll crash.
For more details, see
#24953 (comment)
#24953 (comment)
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.
Ok I see, I guess we have no other option then
# Conflicts: # src/components/withNavigationFallback.js
Hi, folks. Today, I did some deeper investigation into the relevant logic and came up with three options, please feel free to let me know your thoughts. : )
cc @techievivek |
@ntdiary Thanks a lot for your investigation! But I think that it's better to move forward with this PR and put any other future updates related to the FYI: there is another bug that can be resolved using this HOC, so updates can touch that place as well : |
Reviewer Checklist
Screenshots/VideosAndroid: Native30072-android-native.mp4Android: mWeb Chrome30072-android-chrome.mp4iOS: Native30072-ios-native.mp4iOS: mWeb Safari30072-ios-safari.mp4MacOS: Chrome / Safari30072-web.mp4MacOS: Desktop30072-desktop.mp4 |
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'm happy to approve this migration. : )
Additionally, we can try to remove this component in a separate issue.
As for #30636, maybe we can try to stop relying on this component and mock a navigation in preview.js
Line 22 in 33dbb11
const decorators = [ |
const decorators = [
(Story) => {
const Screen = () => Story();
return (
<ComposeProviders
components={[OnyxProvider, LocaleContextProvider, HTMLEngineProvider, SafeAreaProvider, PortalProvider, EnvironmentProvider, KeyboardStateProvider, WindowDimensionsProvider]}
>
<NavigationContainer>
<StoryBookStack.Navigator>
<StoryBookStack.Screen name="myScreen" component={Screen} options={{header: () => null, cardStyle: {overflow: 'visible'}}} />
</StoryBookStack.Navigator>
</NavigationContainer>
</ComposeProviders>
);
},
];
also need to remove this line:
App/.storybook/webpack.config.js
Line 30 in 33dbb11
'@react-navigation/native': path.resolve(__dirname, '../__mocks__/@react-navigation/native'), |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@ntdiary Your solution looks more fundamental! Also, I liked your idea with mocking Will you prepare a proposal then for #30636? |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Ooops looks like we introduced a performance regression @VickyStash @ntdiary ? |
@techievivek Hm, updates of this PR are pretty straightforward and only TS-related, so it looks strange. |
Yeah, this PR is quite simple and theoretically should not cause performance issues. Also, I've encountered this kind of problem in some previous PRs. Our previous conclusion was that performance tests are not very stable. 😂 |
🚀 Deployed to staging by https://github.com/techievivek in version: 1.3.96-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.96-15 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀
|
Details
[TS migration] Migrate 'withNavigationFallback.js' HOC to TypeScript
Fixed Issues
$ #24953
PROPOSAL: N/A
Tests
This HOC is used to add NavigationContext to the components outside of the NavigationContainer.
First case: Button is used outside of the NavigationContainer in the confirmation popup:
Second case: Composer component in Storybook:
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android.web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4
Storybook
storybook.mp4