add ability to use custom state persistence fn #21
Conversation
| const { | ||
| navigation, | ||
| screenProps, | ||
| persistNavigationState, |
There was a problem hiding this comment.
I'm not sure if this is a good thing, but if you keep the old diff, tests fail
src/createAppContainer.js
Outdated
| if (!persistenceKey) { | ||
| return; | ||
| } | ||
| await AsyncStorage.setItem(persistenceKey, JSON.stringify(navState)); |
There was a problem hiding this comment.
We should probably also move the require statement inside here to avoid the warning from RN. cc @brentvatne
There was a problem hiding this comment.
I added these functions so that the code is backward compatible, but I could as well remove the default value for persistNavigationState, loadNavigationState and the persistenceKey prop, to remove the AsyncStorage dependency altogether. That will be breaking, but in the docs, the api is described as experimental and it is explicitly said the api may change in future, so it's fine, imho.
There was a problem hiding this comment.
yeah i think it's fine to remove it, and in the docs we can provide some default implementation for people that use asyncstorage
src/createAppContainer.js
Outdated
| @@ -227,7 +239,7 @@ export default function createNavigationContainer(Component) { | |||
| startupState = Component.router.getStateForAction(action); | |||
| } | |||
|
|
|||
| // Pull persisted state from AsyncStorage | |||
| // Pull user-provided persisted state | |||
| if (startupStateJSON) { | |||
| try { | |||
| startupState = JSON.parse(startupStateJSON); | |||
There was a problem hiding this comment.
currently loadNavigationState is expected to return a json string and we call JSON.parse on that string here to get an object.
I'd vote for loadNavigationState returning an object instead of a string (It'd just require moving this JSON.parse into the user-provided function).
From a developer perspective, for whatever reason, if I want to manipulate the state, I'd prefer to work with an object and return that object, rather than converting a json string (from asyncStorage) to an object, then manipulate what I need and then stringifying that (which may not work fi I need to insert something non-stringifyable), passing it to react-navigation which would call JSON.parse again. wdyt @brentvatne ?
There was a problem hiding this comment.
I went ahead and did that in 6d9510b
There was a problem hiding this comment.
that seems totally reasonable @vonovak! apologies for the slow reply
|
lgtm! a couple things we can do as followup:
|
vonovak
changed the title
vonovak
changed the title
good point, added in c59cc01 - feel free to change the message if something is missing I made another change in 0fa5e46 that handles the case when I'll open a PR for the docs shortly |
|
lgtm! |
|
let's ship this soon :> should i do it tomorrow @vonovak? |
|
The PR is good for merge, so feel free to ship as you see fit. The docs react-navigation/react-navigation.github.io#425 update is also ready. |
|
it's live! |
motivation: our app, historically, does not always have JSON-serializable navigation params.
Some of our params are instances of classes that, when you do
JSON.stringify(param), the call does not throw, but the resulting JSON loses some information and the original instance cannot be recovered. So it seems like the state was serialized successfully, but after a reload, the screens do not have their correct nav params, causing a crash.I'd like to be able to specify custom serialization fn so that we can serialize only the state we want.
As a side effect, I also added
loadNavigationStatewhich helps remove dependency on AsyncStorage (#17) and allows to modify the recovered nav state object.