Upgrade from redux-persist v4 to redux-persist v5. #2762

roberthoenig · 2018-07-05T20:08:20Z

Please try this out and report how it works for you!

roberthoenig · 2018-07-05T20:09:41Z

(Flow will fail on this. From what I can see this is due to some issues with the redux-persist annotations.)

borisyankov · 2018-07-05T20:20:56Z

src/boot/store.js

@@ -14,17 +15,20 @@ import middleware from './middleware';
 //   rootReducer,
 //   compose(autoRehydrate(), applyMiddleware(...middleware)),
 // );
+const reduxPersistConfigV4 = {
+  whitelist: [...config.storeKeys, ...config.cacheKeys],


Extract to const whitelist = [...config.storeKeys, ...config.cacheKeys] and then reuse in both reduxPersistConfigV4 and reduxPersistConfig.

borisyankov · 2018-07-05T21:07:50Z

src/boot/StoreProvider.js


 type Props = {
  children: ChildrenArray<*>,
 };

 export default class StoreHydrator extends PureComponent<Props> {
-  componentDidMount() {


We would want to keep this metric.
I guess we need to implement it differently.

Looking at this:
https://github.com/rt2zz/redux-persist/blob/c9ae1903c3c5dea8604dd79ceea50c8a0a64f8fd/src/integration/react.js#L38
onBeforeLoad is probably a good place to start the timing, but stopping the timing is not so clearly defined... zi would have hoped for a onAfterLoad.
We can do it on unmounting of the loading component.

borisyankov · 2018-07-10T06:13:41Z

src/diagnostics/Timer.js

+  render() {
+    return this.props.children;
+  }
+}


Cool. That looks correct just looking at the code.
We might want to name this more descriptively. 'RenderTimer' sounds like an improvement. Any other alternatives you can come up with?

roberthoenig · 2018-07-11T14:22:38Z

Renamed Timer to RenderTimer.

roberthoenig · 2018-07-11T17:53:13Z

Updated to apply the compression first and then the upgrade to v5. NOT TESTED YET

borisyankov · 2018-07-19T12:53:18Z

Hydration on master on my S8 Plus takes 940msec on subsequent starts (not initially empty-store one)

Currently this PR is crashing on my phone, but will be debugging.

roberthoenig · 2018-07-19T12:55:36Z

Note that this PR is WIP, and I haven't pushed to it for a while so I'm expecting it to crash.

zulipbot · 2018-07-30T15:25:47Z

Heads up @roberthoenig, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

roberthoenig · 2018-08-02T17:55:03Z

Updated: Split into multiple commits.

borisyankov · 2018-08-02T20:14:21Z

src/boot/store.js

+// of the code that used redux-persist v4. This config object is used for
+// migrating any device that is ugprading from an app version that uses
+// redux-persist v4 to an app version that uses redux-persist v5.
+const reduxPersistConfigV4 = {


You probably want to move this to a migrations file?

zulipbot · 2018-08-03T05:01:02Z

Heads up @roberthoenig, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

gnprice

Thanks!

I've just gone and pushed the first few changes here, that are making preparatory changes that are easy to merge now. I reframed them a bit in the commit messages, and also split up the changes slightly differently -- take a look at 6f703dc and ancestors:

* 6f703dc04 rehydration: Allow `payload` to be undefined.
* 5882d0af2 ZulipAsyncStorage: Handle undefined callback.
* 63739998e ZulipAsyncStorage: Handle null result in getItem.

In particular, the two in ZulipAsyncStorage are really fixing bugs that were there already (we aren't fully providing the API of AsyncStorage) and we just hadn't hit with v4. In each change we're extending what we have to allow for more cases, so the summary line focuses on how we're extending it; the body of the commit message then adds the context that this is motivated by redux-persist v5.

gnprice · 2018-08-03T04:42:01Z

src/actionTypes.js

@@ -107,7 +107,7 @@ import type {
 */
 export type RehydrateAction = {
  type: typeof REHYDRATE,
-  payload: GlobalState | { accounts: null } | {},
+  payload: GlobalState | { accounts: null } | void,


This can still be {} at least before the migration, so I'll leave that possibility in. Could remove it later if we have a reason to be confident it can't be anymore.

The rehydration mechanism changes with redux-persist v5, in a way that is incompatible with the current timing mechanism. To keep the upgrade to v5 easy, remove the timing code now. It'll be readded in a compatible form after the upgrade.

redux-persist v5 introduces a couple breaking changes. These are described in detail in https://github.com/rt2zz/redux-persist/blob/master/docs/MigrationGuide-v5.md Here are some of the changes that affect our code (other changes have already been addressed in the previous commits): * v5 auto-rehydrates the state by default, so we don't need to call autoRehydrate() explicitly anymore. * v5 dispatches different actions during the first rehydration. This makes it incompatible with `redux-action-buffer`, because redux-action-buffer silently buffers all these actions which prevents the redux-dispatch init from completing. Conveniently, v5 also ships with a new component, `PersistGate`, that buffers all non-redux-persist actions and can also display our loading screen while the state is getting initializeed. Therefore, get rid of `redux-action-buffer` and replace it with `PersistGate`. * v5 uses a different storage format that is incompatible with the old format. Automatically convert the old format to the new format with getStoredStateMigrateV4(). * v5 has internal Flow errors, so exclude the module from flow and add a type stub with flow-typed. The latest official release of redux-persist is v5.7.0, and the latest version is v5.10.0. The code changes between v5.7.0 and v5.10.0 are mostly bugfixes and no bigger features, so use v5.10.0.

Previously, redux-persist v4 stored each of the top-most subtrees of the state under a separate key in the database. By default, v5 stores the whole state under a single key that we specify ('root'). Add a migration to remove the old keys from the database.

The store hydration timer got removed in bf659a2 when we upgraded from redux-persist v4 to v5 due to structural differences in the redux initialization. Re-add a timer. Do this with a new, reusable Timer component that logs the duration of its existence to timing.js. This works for roughly measuring the redux-persist execution time, because that's all the app is doing while the loading screen is shown. No other big actions distort this measurement, because no significant actions are dispatched as long as the state is not properly loaded.

@roberthoenig

This is a pure refactor that just reorganizes the code slightly. This version is perhaps cleaner in itself; but what really motivates the change is to make it look more like what we'll have for using redux-persist v5, to shrink the upcoming diff that does that. Based on @roberthoenig's drafts in #2762 of the redux-persist upgrade.

zulipbot · 2018-08-03T07:40:52Z

Heads up @roberthoenig, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

roberthoenig · 2018-08-03T09:20:00Z

Closing this PR:

As discussed on chat.zulip.org, the way redux-persist v5 stores the state, along with the incomplete migration system, makes it look easier to just stick with redux-persist v4.

The plan now is to harness the benefits of a migration system by using redux-persist-migrate with ships as an integration for v4. Then, we are going to maintain our own fork of redux-persist, so we can cherry-pick important commits off of master as needed.

redux-persist-migrate allows us to change our state structure while staying compatible with older app versions. We check-in the integration directly into our codebase because it's only 60 LOC and thus easily maintainable. Furthermore, upstream development now happens directly in redux-persist v5, which we won't use. For the reasoning behind not using v5, see zulip#2762 Note that this commit doesn't pass CI on its own, because it is not compatible with ESLint -- the code is taken as is from https://github.com/wildlifela/redux-persist-migrate/blob/e74b905a5d1a91931db9fd10b1a73a44c03f0559/src/index.js The following commit will make the code ESLint compatible. We do this in a separate commit so that the modifications to the original library are easily diffable. (Fwiw, the code added in this commit is not 100% identical to the code in redux-persist-migrate, because of the automatic prettier formatting on save in VSCode.)

redux-persist-migrate allows us to change our state structure while staying compatible with older app versions. We check the redux-persist-migrate directly into our codebase because it's only 60 LOC and thus easily maintainable. Furthermore, upstream development of redux-persist-migrate now happens in redux-persist v5, which we decided not to use. To see why we don't use v5, see zulip#2762 Note that this commit doesn't pass CI on its own, because it is not compatible with ESLint -- the code is taken as is from https://github.com/wildlifela/redux-persist-migrate/blob/e74b905a5d1a91931db9fd10b1a73a44c03f0559/src/index.js The following commit will make the code ESLint compatible. We do this in a separate commit so that the modifications to the original library are easily diffable.

redux-persist-migrate allows us to change our state structure while staying compatible with older app versions. We check the redux-persist-migrate source directly into our codebase because it's only 60 LOC and thus easily maintainable. Furthermore, upstream development of redux-persist-migrate now happens in redux-persist v5, which we decided not to use. To see why we don't use v5, see: #2762 Note that this commit doesn't pass CI on its own, because it is not compatible with ESLint -- the code is taken as is from https://github.com/wildlifela/redux-persist-migrate/blob/v5.0.0/src/index.js The following commit will make the code ESLint compatible. We do this in a separate commit so that the modifications to the original library are easily diffable. The license identification in THIRDPARTY is taken from the author's package.json file. [greg: Added THIRDPARTY stanza. Also created that file just now, in a previous commit, to have a place to put that. :-) ]

redux-persist-migrate allows us to change our state structure while staying compatible with older app versions. We check the redux-persist-migrate directly into our codebase because it's only 60 LOC and thus easily maintainable. Furthermore, upstream development of redux-persist-migrate now happens in redux-persist v5, which we decided not to use. To see why we don't use v5, see zulip#2762 Note that this commit doesn't pass CI on its own, because it is not compatible with ESLint -- the code is taken as is from https://github.com/wildlifela/redux-persist-migrate/blob/e74b905a5d1a91931db9fd10b1a73a44c03f0559/src/index.js The following commit will make the code ESLint compatible. We do this in a separate commit so that the modifications to the original library are easily diffable.

In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even with compression, we're within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store the non-serializable ZulipVersion in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, given the above, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there as well (it has the needed type definitions). Then, address redux-persist's two dependencies that weren't listed in our own package.json: `json-stringify-safe` and `lodash`. For the former, just add it to package.json. For the latter, note that the only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.

In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even after compression, our entire state tree would be within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store non-serializable data (such as a ZulipVersion) in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there too; it has the needed type definitions. Then, address redux-persist's two dependencies that weren't listed in our own package.json: - `json-stringify-safe`: Add to package.json. - `lodash`: The only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make this change, the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.

borisyankov reviewed Jul 5, 2018

View reviewed changes

borisyankov reviewed Jul 10, 2018

View reviewed changes

roberthoenig force-pushed the 21 branch 2 times, most recently from 0a6f037 to 157ffbc Compare July 11, 2018 14:21

roberthoenig force-pushed the 21 branch 2 times, most recently from a95b545 to c98eb6f Compare July 11, 2018 17:43

roberthoenig changed the title ~~Upgrade redux-persist and compress the storage.~~ [WIP]Upgrade redux-persist and compress the storage. Jul 19, 2018

roberthoenig force-pushed the 21 branch from c98eb6f to e7610ca Compare August 1, 2018 17:03

roberthoenig changed the title ~~[WIP]Upgrade redux-persist and compress the storage.~~ Upgrade from redux-persist v4 to redux-persist v5. Aug 1, 2018

roberthoenig force-pushed the 21 branch 2 times, most recently from e00c7d1 to 0fc8f50 Compare August 2, 2018 17:54

borisyankov reviewed Aug 2, 2018

View reviewed changes

gnprice reviewed Aug 3, 2018

View reviewed changes

roberthoenig added 4 commits August 2, 2018 22:02

diagnostics: Don't time rehydration (for now).

7a4cac9

The rehydration mechanism changes with redux-persist v5, in a way that is incompatible with the current timing mechanism. To keep the upgrade to v5 easy, remove the timing code now. It'll be readded in a compatible form after the upgrade.

gnprice force-pushed the 21 branch from 0fc8f50 to b5c9938 Compare August 3, 2018 05:06

roberthoenig closed this Aug 3, 2018

chrisbobbe mentioned this pull request Apr 17, 2020

[REVIEWABLE] A second approach to storing non-serializable data in Redux #4047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade from redux-persist v4 to redux-persist v5. #2762

Upgrade from redux-persist v4 to redux-persist v5. #2762

roberthoenig commented Jul 5, 2018

roberthoenig commented Jul 5, 2018

borisyankov Jul 5, 2018

borisyankov Jul 5, 2018

borisyankov Jul 9, 2018

borisyankov Jul 10, 2018

roberthoenig commented Jul 11, 2018

roberthoenig commented Jul 11, 2018

borisyankov commented Jul 19, 2018

roberthoenig commented Jul 19, 2018

zulipbot commented Jul 30, 2018

roberthoenig commented Aug 2, 2018

borisyankov Aug 2, 2018

zulipbot commented Aug 3, 2018

gnprice left a comment

gnprice Aug 3, 2018

zulipbot commented Aug 3, 2018

roberthoenig commented Aug 3, 2018

Upgrade from redux-persist v4 to redux-persist v5. #2762

Upgrade from redux-persist v4 to redux-persist v5. #2762

Conversation

roberthoenig commented Jul 5, 2018

roberthoenig commented Jul 5, 2018

borisyankov Jul 5, 2018

Choose a reason for hiding this comment

borisyankov Jul 5, 2018

Choose a reason for hiding this comment

borisyankov Jul 9, 2018

Choose a reason for hiding this comment

borisyankov Jul 10, 2018

Choose a reason for hiding this comment

roberthoenig commented Jul 11, 2018

roberthoenig commented Jul 11, 2018

borisyankov commented Jul 19, 2018

roberthoenig commented Jul 19, 2018

zulipbot commented Jul 30, 2018

roberthoenig commented Aug 2, 2018

borisyankov Aug 2, 2018

Choose a reason for hiding this comment

zulipbot commented Aug 3, 2018

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 3, 2018

Choose a reason for hiding this comment

zulipbot commented Aug 3, 2018

roberthoenig commented Aug 3, 2018