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

Keys with value `undefined` don't survive round-trips into JSON, and the zulipVersion will be persisted in JSON. Use `null`, to maintain that Account objects always have the same number of keys, and we don't have to worry about the distinction between an optional `string` property and a property that's `string | void`. The same strategy is in use for the ackedPushToken. As a side-effect, it's much simpler to distinguish between the second and third behaviors of `makeAccount` in the list below, for tests. We don't need the `shouldHaveZulipVersion` param anymore: 1. Supply our own valid string value for the `zulipVersion` field in the returned Account object 2. Make the returned Account object have an empty `zulipVersion` 3. Make the returned Account object have a default, valid string value for `zulipVersion`

…ll`.

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.

See the previous commit for motivation.

Incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b so that we can pass custom serialize/deserialize functions for replace/revive logic, to eventually store non-serializable data (such as a ZulipVersion; that's this series) in Redux.

In an upcoming commit, we will set up a system of "replacer" and "reviver" transforms so that we can store non-serializable objects, such as ZulipVersion instances, in Redux, and have a serializable format be persisted to disk. This same system will be used for non-serializable objects besides ZulipVersion instances; see zulip#3949 and zulip#3950, which will be much easier after this series. We use our own fork of `remotedev-serialize`, at 5f9f759a4, which Greg authored atop `zalmoxisus/remotedev-serialize.git` at 0.1.8, because it fixes an important bug. Also, the project seems unmaintained, and the maintainer hasn't been active anywhere on GitHub. The bug is that data fails to round-trip when it contains a "__serializedType__" key, which is used by the reviver to identify and reverse a particular replace transform. Such data, if present, could trigger hidden functionality. Also, explain the process of getting the `remotedev-serialize` libdef set up, in docs/howto/libdefs.md.

`null` is passed as the `refs` param in an example in the official docs: https://github.com/zalmoxisus/remotedev-serialize#passing-custom-serialization-functions

This sets up how we'd like to plug in custom replacers and revivers, but doesn't add any yet. We can use the '__serializedType__' string freely because we're using `zulip/remotedev-serialize@5f9f759a4`, which contains a fix for proper round-tripping of that string, to avoid collisions when that string occurs as a key representing data, and the unplanned behavior such collisions would cause.

No ZulipVersion instances are stored in Redux in this commit; this just handles the replace/revive logic for when they are, in an upcoming commit.

Storing the parsed ZulipVersion instance in Redux makes it easier to work with, so we don't have to think about constructing a new instance as part of every server feature check. The one tricky bit is that our Redux state is serialized by redux-persist so that it can be stored in ZulipAsyncStorage, but the ZulipVersion instance itself is not serializable. Thankfully, it can be fully represented by its raw version string, which is serializable. So, on entry into ZulipAsyncStorage, we just have to convert the ZulipVersion instance into the raw version string, with .raw() ("replace" it), and on exit, make a new ZulipVersion instance, by calling the constructor with that raw version string ("revive" it). We considered two main strategies for locating the bit of state to be transformed (in this case, the `zulipVersion` field, which stores a ZulipVersion value, in elements of the `state.accounts` array): 1) The "outside-in" strategy, of identifying the value by the extrinsic property of where it is; i.e., that it's at the field named 'zulipVersion' on elements of the `state.accounts` array, or 2) The "inside-out" strategy, of identifying the value by its intrinsic property of being `instanceof ZulipVersion`. We chose the latter. When we work on zulip#3950, converting our object-as-map fields to use Map or Immutable.Map, we'll be making similar, sweeping changes to many different parts of the state, so it's most natural for the bulk of our logic to be independent of the location in state, and focus instead on the type of non-serializable object being stored. This approach conveniently clears the path to use ImmutableJS for additional reasons discussed later. An exploration of the "outside-in" approach is left as the un-merged PR zulip#3952. The main advantage of that approach is that we wouldn't need to write migrations, but it had the significant drawback of requiring a lot of code (for locating the bit of state to be transformed) that was 1) boilerplate, and 2) difficult to get fully type-checked, which is a bad combination. These concerns were not sufficiently alleviated by the complexity of that boilerplate being bounded by the complexity of the reducer(s) in charge of the corresponding part(s) of the state: it's better just to not have to write the boilerplate. There's nothing stopping us from mixing the two approaches, in future, but it would be nice to stick to one as far as possible, for simplicity. For the "inside-out" implementation, we use `remotedev-serialize` (added in a recent commit) [1], with custom replacer and reviver functions, defined in the previous commit. As Greg explains at zulip#3952 (comment): """ * at save time, values that are `instanceof ZulipVersion` get turned into objects like `{__serializedType__: 'ZulipVersion', data: '2.0.0-rc1'}`, before being JSON-serialized * at load time, after JSON deserialization, values that are objects with a `__serializedType__` property get transformed in a way identified by that property's value: in particular if `__serializedType__: 'ZulipVersion'`, then `data` is fed to `new ZulipVersion` """ Since we've never dealt with the Zulip version in this "live" (i.e., non-serializable) form, we have to write a migration. This was straightforward, but we MUST remember to write this kind of migration in the future. For making a value "live", where it wasn't before, the migration needs to: 1) As input, take the previous shape of the data. Don't confuse this with the *current* way of storing the "dead" shape. Just like any other migration, the previous shape is the input. 2) As output, give the "live" form of the data. Once it's in Redux, the replacer will take care of persisting it in the correct "dead" form. As mentioned above, this approach further clears the way for ImmutableJS; zulip#3949 and zulip#3950 will be much easier after this. I've neglected to mention that the primary purpose of `remotedev-serialize` is to replace and revive ImmutableJS objects! Its "default" replacers and revivers do this. The custom replacer/reviver functions we use here are a nice feature provided on the side. (We added `immutable` as a dependency in a recent commit, since `remotedev-serialize` depends on it.) Fixes: zulip#3951 [1]: We actually use our own fork, `zulip/remotedev-serialize@5f9f759a4`, which fixes a bug where "__serializedType__" keys in data fail to round-trip properly.

It may feel smoother not to have to call the constructor when doing server version checks.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Commits on May 5, 2020