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

State Persistence #3716

Merged
merged 5 commits into from Mar 23, 2018

Conversation

Projects
None yet
5 participants
@ericvicenti
Contributor

ericvicenti commented Mar 11, 2018

Knocking down a big item on the v2 roadmap!!

Adds a new state persistence mechanisms to all of the navigators via createNavigationContainer

There are two new props that you can provide to a navigation container: storageKey and renderLoading.

storageKey is the string key to be used with AsyncStorage while saving and persisting navigation state. To disable persistence, set to null.

renderLoading allows you to render something while the navigator re-hydrates its state and loads its initial URL. By default this returns null, but Expo users will want to render a component for smooth app launches

There is also functionality in this PR to observe errors that come from re-hydrating state, and gracefully recover by dispatching an init action.

Also this revises the init action to reset the navigation state, rather than preserve the previous state.

@react-navigation-ci

This comment has been minimized.

}
_getStorageKey() {
// a null storageKey prop tells the container to NOT persist, so we explicitly check for undefined
if (this.props.storageKey === undefined) {

This comment has been minimized.

@brentvatne

brentvatne Mar 12, 2018

Member

I think state persistence should be opt-in rather than opt-out -- more apps will not use it than those that will.

This comment has been minimized.

@ericvicenti

ericvicenti Mar 14, 2018

Contributor

sure. I'll rename the prop to persistenceStorageKey and make it default to null

dispatch = action => {
if (!this._isStateful()) {
return false;
dispatch = async action => {

This comment has been minimized.

@brentvatne

brentvatne Mar 12, 2018

Member

I don't think we should change this to be async, we depend on it returning true or false in some situations and users might as well.

This comment has been minimized.

@ericvicenti

ericvicenti Mar 14, 2018

Contributor

good point, will fix

@brentvatne brentvatne referenced this pull request Mar 12, 2018

Closed

React Navigation 2.0 roadmap #3685

24 of 35 tasks complete

@ericvicenti ericvicenti force-pushed the @ericvicenti/persistence branch from 9e27853 to 1d6c6e4 Mar 15, 2018

@react-navigation-ci

This comment has been minimized.

asddressed

@ericvicenti ericvicenti force-pushed the @ericvicenti/persistence branch from 1d6c6e4 to 95d19cf Mar 15, 2018

@react-navigation-ci

This comment has been minimized.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Mar 15, 2018

we might want to flag this as experimental initially because I imagine that we will iterate on the renderLoading api. for example, if you already have some loading screen rendered for another step in app loading process, you will want to keep it rendered rather than render the navigator with the renderLoading api. so we may want to have some way to imperatively load data so we have the data available when we mount navigator and can then do it synchronously like usual.

similarly, we might want to hook into loading error and completion events with callbacks, or maybe we want to pass that state into the renderLoading fn so it can react accordingly. unclear to me what the best approach is at this point

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Mar 15, 2018

Yeah, how would you like to mark this as experimental?

@brentvatne

This comment has been minimized.

Member

brentvatne commented Mar 15, 2018

maybe something like this?

persistenceExperimental={{
  storageKey: '..',
  renderLoading: () => <View />
}}
@brentvatne

This comment has been minimized.

Member

brentvatne commented Mar 15, 2018

or

  persistenceKeyExperimental='..',
  renderLoadingExperimental={() => <View />}
@brentvatne

This comment has been minimized.

Member

brentvatne commented Mar 15, 2018

or

  persistenceKey='..'
  renderLoadingExperimental={() => <View />}
@jamsch

This comment has been minimized.

jamsch commented Mar 15, 2018

Just a note: React Native has always had a bug with AsyncStorage.get() when remote debugging is enabled. It only works the first time the app is launched on remote debugging and subsequent refreshes won't work.

State persistence
Adds a new state persistence mechanisms to all of the navigators via createNavigationContainer

    There are two new props that you can provide to a navigation container: `storageKey` and `renderLoading`.

    `storageKey` is the string key to be used with AsyncStorage while saving and persisting navigation state. To disable persistence, set to null.

    `renderLoading` allows you to render something while the navigator re-hydrates its state and loads its initial URL. By default this returns null, but Expo users will want to render a <AppLoading /> component for smooth app launches

    There is also functionality in this PR to observe errors that come from re-hydrating state, and gracefully recover by dispatching an init action.

    Also this revises the init action to *reset* the navigation state, rather than preserve the previous state.

@ericvicenti ericvicenti force-pushed the @ericvicenti/persistence branch from 95d19cf to adf61b3 Mar 16, 2018

@react-navigation-ci

This comment has been minimized.

@react-navigation-ci

This comment has been minimized.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 19, 2018

Codecov Report

Merging #3716 into master will decrease coverage by 0.84%.
The diff coverage is 40.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3716      +/-   ##
==========================================
- Coverage    71.1%   70.25%   -0.85%     
==========================================
  Files          53       53              
  Lines        1623     1661      +38     
==========================================
+ Hits         1154     1167      +13     
- Misses        469      494      +25
Impacted Files Coverage Δ
src/createNavigationContainer.js 51.58% <40.9%> (-7.51%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee1b597...3369742. Read the comment docs.

@react-navigation-ci

This comment has been minimized.

@react-navigation-ci

This comment has been minimized.

@react-navigation-ci

This comment has been minimized.

@brentvatne brentvatne merged commit 9cf557b into master Mar 23, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@ericvicenti ericvicenti deleted the @ericvicenti/persistence branch Mar 25, 2018

@hawkrives hawkrives referenced this pull request May 7, 2018

Merged

React Navigation 2.0 #2574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment