Skip to content
This repository has been archived by the owner on May 7, 2023. It is now read-only.

migration: react navigation v5 #45

Merged
merged 7 commits into from
Nov 2, 2019
Merged

Conversation

hyochan
Copy link
Owner

@hyochan hyochan commented Nov 2, 2019

Description

react-navigation@next package which is currently v5 has majority updates which make the code looks much pretty and concise.

Originally, params such as navigationOption were statically bound which sometimes occurs unexpected behavior like when you try to communicate with header and screen. Looking at v4 and v5, v5 has additional route param that works as a HOC pattern which is much react friendly. In this case, v4 can't ensure that when the getParam is released or is not bound from the child screen. It just doesn't look right to go.

We are hoping not to drive our codes like this even v5 is still in alpha.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • Run yarn test or yarn test -u if you need to update snapshot.
  • Run yarn lint
  • I am willing to follow-up on review comments in a timely manner.

@hyochan hyochan self-assigned this Nov 2, 2019
@hyochan hyochan added this to the Beta Release milestone Nov 2, 2019
@codecov
Copy link

codecov bot commented Nov 2, 2019

Codecov Report

Merging #45 into master will decrease coverage by 0.84%.
The diff coverage is 91.83%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   85.14%   84.29%   -0.85%     
==========================================
  Files          28       32       +4     
  Lines         673      707      +34     
  Branches       83       81       -2     
==========================================
+ Hits          573      596      +23     
- Misses         79       91      +12     
+ Partials       21       20       -1

@hyochan
Copy link
Owner Author

hyochan commented Nov 2, 2019

@marsinearth Please check this! I've got this first hand.

@hyochan hyochan merged commit 4d3c858 into master Nov 2, 2019
@hyochan hyochan changed the title [WIP] migration: react navigation v5 migration: react navigation v5 Nov 2, 2019
@hyochan hyochan deleted the migration/react-navigation-v5 branch November 2, 2019 18:11
@taejs
Copy link

taejs commented Nov 5, 2019

Question: Is SwitchNavigator is deprecated?
Migrating my project to react-navigation-v5, I convert react-navigation/switchNavigator to @react-navigation/compat/switchNavigator following migration. docs

But you converted switchNavigator to RootNavigator that you made with StackNavigator
So I wondered switchNavigator is deprecated

@hyochan
Copy link
Owner Author

hyochan commented Nov 5, 2019

@Ta2Rim Thanks for the detail. Looks like I've missed this in the doc and thanks for the update 👍
Would you give us an effort to add SwitchNavigator in hackatalk?

@taejs
Copy link

taejs commented Nov 5, 2019

@hyochan I'd love to

@taejs
Copy link

taejs commented Nov 5, 2019

How can I work on it?
I don’t know what branch I have to checkout

@hyochan
Copy link
Owner Author

hyochan commented Nov 5, 2019

@Ta2Rim PR is available on master branch. You can refer to the clip

@taejs
Copy link

taejs commented Nov 7, 2019

@hyochan
I tried, but it doesn't work with these errors.

Warning: Can't perform a React state update on an unmounted component. This is a no-op, 
but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in %s.%s, the componentWillUnmount method,
    in SceneView (at TabView.tsx:165)
    in RCTView (at View.js:45)
    in View (at createAnimatedComponent.js:217)
    in AnimatedComponent (at Pager.tsx:667)
    in PanGestureHandler (at Pager.tsx:659)
    in Pager (at TabView.tsx:126)
    in RCTView (at View.js:45)
    in View (at TabView.tsx:125)
    in TabView (at MaterialTopTabView.tsx:152)
    in MaterialTopTabView (at createMaterialTopTabNavigator.tsx:43)
    in MaterialTopTabNavigator (at MainTabNavigator.tsx:101)
    in MainTabNavigator (at CompatScreen.tsx:27)
    in ScreenComponent (at createCompatNavigatorFactory.tsx:139)
    in StaticContainer
    in StaticContainer (at SceneView.tsx:85)
    in EnsureSingleNavigator (at SceneView.tsx:84)
    in SceneView (at useDescriptors.tsx:114)
    in SwitchNavigator (at createCompatNavigatorFactory.tsx:153)
    in Navigator (at MainSwitchNavigator.tsx:40)
- node_modules\react-native\Libraries\YellowBox\YellowBox.js:59:8 in error
- node_modules\expo\build\environment\muteWarnings.fx.js:27:24 in error
- ... 13 more stack frames from framework internals

I guess it's caused that switchNavigator still not support React Hooks.
so I think It's better to use stackNavigator instead of switchNavigator.

image

@hyochan
Copy link
Owner Author

hyochan commented Nov 8, 2019

@Ta2Rim Thanks for trying out! I've also tried myself but could not achieve it either. Let's stick to StackNavigator for now!

hyochan added a commit to daheeahn/hackatalk-mobile that referenced this pull request Nov 17, 2019
* [WIP] Migrated `AuthStackNavigator`
* [WIP] Reseting navigation to [MainStack]
* Ported [MainTab] in [MainStack]
* Updated all test codes
   - Reorganized testUtils mainly.
* Remove [SwitchNavigator]
* Remove old and dead codes
* Fix navigation types
@hyochan
Copy link
Owner Author

hyochan commented Nov 24, 2019

@Ta2Rim I have news for you in my post. The workaround for SwitchNavigator is still unreliable.

@taejs
Copy link

taejs commented Nov 24, 2019

@hyochan
Thanks! It's very informative.
I wonder what maintainers intended

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants