[WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. #3952

chrisbobbe · 2020-03-04T22:11:53Z

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 with
JSON.stringify by redux-persist so that it can be stored in
ZulipAsyncStorage, but the ZulipVersion instance itself is not
serializable. Thankfully, it can be represented as its raw version
string, which is serializable.

So, on entry into ZulipAsyncStorage, convert the ZulipVersion
instance into the raw version string with .raw(), and on exit, call
the ZulipVersion constructor on that raw version string, with
redux-persist's createTransform.

Fixes: #3951

chrisbobbe · 2020-03-04T22:23:32Z

My first question on this is here.

Also, I wonder about all my imports of the ZulipVersion class, particularly where I only use it as a type, like in src/actionTypes.js. It's the first import of runtime code in that file (import... instead of import type...); is this a pattern we'd like to avoid?

rk-for-zulip

My first question on this is here.

Assuming you mean

The other way we could go is more general; we apply the inbound and outbound transforms to all fields named zulipVersion that appear anywhere in the state.`

then (as implied by one of my comments below) that's definitely not a thing we want to do: we have a number of maps with (almost?) arbitrary string keys, which might well have a field zulipVersion.

Also, I wonder about all my imports of the ZulipVersion class, particularly where I only use it as a type, like in src/actionTypes.js. It's the first import of runtime code in that file (import... instead of import type...); is this a pattern we'd like to avoid?

It is, yes. I believe import type { ZulipVersion } from ... will work just as well in that case.

rk-for-zulip · 2020-03-05T18:38:21Z

src/boot/store.js

+      zulipVersion:
+        // $FlowMigrationFudge
+        a.zulipVersion instanceof ZulipVersion ? a.zulipVersion : new ZulipVersion(a.zulipVersion),


This $FlowMigrationFudge is covering an actual type error. If the account does not have a zulipVersion, this transformation will try to instantiate new ZulipVersion(undefined). The migration needs to retain the property of "not having a zulipVersion field at all".

In fact, I'm pretty sure the migration should be the identity function! The on-disk form of the data stored is identical in the old and new worlds – we're (still) storing zulipTransform as the original raw string.

Good catch, thanks!

src/boot/store.js

rk-for-zulip · 2020-03-05T19:01:56Z

src/boot/store.js

+ */
+const zulipVersionTransform = createTransform(
+  // Only applies to the `zulipVersion` field in state.accounts.
+  (inboundState, key) => {


(It doesn't help matters that their choice of definition of "in" and "out" are the opposite polarity I was expecting.)

Me too 😉

Happily this name belongs to a parameter, so the library's choice for it is effectively just documentation -- we're free to call it what we like. "storageBoundState" vs "reduxBoundState"? "writingState" vs "readingState"?

rk-for-zulip · 2020-03-05T19:04:36Z

src/boot/store.js

+    }
+    const result = inboundState.map(account => {
+      if (account.zulipVersion === undefined) {
+        return { ...account, zulipVersion: undefined };


Type error: this should just be return account;.

src/boot/store.js

rk-for-zulip · 2020-03-05T19:12:30Z

src/boot/store.js

+      if (account.zulipVersion === undefined) {
+        return undefined;
+      }


Far worse type error: this should also be return account;. (As written, it will lose the entire account!)

chrisbobbe · 2020-03-05T22:21:10Z

Thanks for the review, @ray-kraesig!

we have a number of maps with (almost?) arbitrary string keys, which might well have a field zulipVersion

In particular, this is a strong reason to go straight to createTransform instead of doing it through remotedev-serialize, in addition to the reason I mentioned (see the motivation for this WIP PR here).

What we're left with is a difficulty in having a system that's agnostic to the particular way ZulipVersion instances are currently stored in Redux (i.e., in objects in the state.accounts array). Whenever the structure changes, we'll have to update the transformers. This is probably OK, but I think a failure to update the transformers would lead to silent bugs; JSON.stringify doesn't throw an error if you give it an object with functions in it (e.g., JSON.stringify({ a: () => {} }); gives "{}"). So we'll want to do careful testing to try and catch that.

Thoughts, @gnprice?

gnprice · 2020-03-05T23:30:44Z

I agree that it'll be better to do this in a shape that operates on a particular spot in the structure, like this prototype, rather than looking for keys anywhere with the name zulipVersion.

If we wanted to do this with remotedev-serialize, that would actually be a third strategy, though. The remotedev-serialize strategy (implemented for Immutable.Map in the bits of code linked to from #3949) would be:

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

For this to work, naturally it's important that you don't have any objects that accidentally have __serializedType__ as a key. I think that's actually true for us: the keys in our objects-as-maps all live in well-defined namespaces, namely numbers, emails, comma-separated numbers or emails, and JSON-serialized narrows. This is one of the good reasons not to expand our use of objects-as-maps and instead to work to reduce it. 🙂

gnprice · 2020-03-05T23:33:20Z

I could also see us deciding that this strategy, of transforming particular parts of the state tree based on where they're found, is what we want to do for handling ZulipVersion and also for handling Map and Immutable.Map, instead of using remotedev-serialize.

Within this strategy, I think there are two main things I'd like to refine starting from here:

The code that knows about the detailed layout of state.accounts -- about the fact that it's a list of objects, each with (possibly) a zulipVersion property which should be converted to and from ZulipVersion -- I'd like to keep together with the other code that knows about the detailed layout of state.accounts. (Much like Consolidate internal details of each of our Redux data structures #3934.) Short of some broader refactor (like discussed in Consolidate internal details of each of our Redux data structures #3934), the most natural spot for that is probably in accountsReducer.js.

Then these transform functions might look something like:

  (savingState, key) => {
    switch (key) {
      case 'accounts':
        return accountsTransformSave(savingState);
      default:
        return savingState;
    }
  }

(The migration functions feel different to me, more OK to centralize like this -- perhaps partly because they aren't relevant to the steady state of the app, while these transform functions are.)

As Ray says, let's get some types in here. 😄

JSON.stringify doesn't throw an error if you give it an object with functions in it

Our JSONable type should help take care of that.

A good step would be to apply AccountsState at one end and JSONable at the other. That'll require some fixmes in between, because JSONable is a lot more general, but those explicit fixmes will be better than the anys that currently apply to the whole thing.

Then a possible step toward getting reasonably solid type-checking would be to actually write down the type of "AccountsState as converted to JSONable". It'll look something like {| ...Account, zulipVersion: string |}[]. Then we can make that and AccountsState the input and output of the accounts-specific transform functions (one of them each way around).

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 with JSON.stringify 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, convert the ZulipVersion instance into the raw version string with .raw(), and on exit, call the ZulipVersion constructor on that raw version string. We use redux-persist's `createTransform`. We locate a value to apply this transformation to with an "outside-in" approach, in particular, by the value's extrinsic property of where it is in the Redux store. That is, we know that it's at the `zulipVersion` key on all Account objects in state.accounts. Instead, we might have used an "inside-out" approach to locate a value by its *intrinsic* property of being a ZulipVersion instance. (We don't insist on using a single approach for all future transforms, but it's useful to compare the two because it's possible that some kinds of transformations fit one way better than the other. See zulip#3950, "Use Map rather than object-as-Map in our Redux state", and zulip#3949, "Use good data structures in Redux state, to avoid quadratic work", for more things we'll need to transform.) The "outside-in" approach requires that we write and maintain code to establish the location of the values to transform. But the complexity of that code will be no more than the complexity of the reducer(s) in charge of the same part(s) of the state, so the burden isn't worrisome. Establishing the location is a two-step process: 1) In src/boot/store, tell the redux-persist transform, created with createTransform, what key you want to handle. (In this case, 'accounts'.) 2) In the reducer file corresponding to that key, write "save" and "load" functions that know where to find the value. (They also do the actual transformation.) It's helpful in step 2 to consolidate code into the reducer files, which already have code that's aware of the structure of the part of state being treated. (Further consolidation like this is zulip#3934.) If we had instead gone with the "inside-out" approach (i.e., locating the value by its being `instanceof ZulipVersion`), which doesn't depend on knowing the value's location in the Redux state, we might have used a handy feature of `remotedev-serialize` called "custom serialization functions" (see https://github.com/zalmoxisus/remotedev-serialize#passing-custom-serialization-functions). As noted in zulip#3949, at some point we'll probably use remotedev-serialize for the particular purpose of transforming Immutable.js data structures, so it may be convenient to piggy-back on that for this custom functionality. Greg explains that strategy 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` """ This would eliminate the need for custom code that tracks the location of the values to be transformed. Reasons we don't use this strategy here are that we don't expect the location of the ZulipVersion value to change, and that the remotedev-serialize strategy always requires writing a migration; old data has never been persisted with that __serializedType__ property. Fixes: zulip#3951

gnprice · 2020-04-16T02:08:20Z

@chrisbobbe , you wrote over at #3745 (comment) referring to this PR:

I believe I've addressed the most recent review

Copying that here, because otherwise this thread looks to me like it's waiting for the next action from you 🙂 (A comment is a good way to say "this is ready for another review now".)

Also, do you still want the "WIP" in the title?

chrisbobbe · 2020-04-16T02:36:34Z

@chrisbobbe , you wrote over at #3745 (comment) referring to this PR:

I believe I've addressed the most recent review

Copying that here, because otherwise this thread looks to me like it's waiting for the next action from you 🙂 (A comment is a good way to say "this is ready for another review now".)

Ah, right — sorry about that, I should indeed have commented. And it looks like I do need to resolve some merge conflicts; I'll do that tomorrow.

Also, do you still want the "WIP" in the title?

This is a more complete version of the work toward this strategy, but I think the [WIP] is still important until we decide whether we might actually want to go with the other approach:

(from a commit message in this revision)

the "inside-out" approach (i.e.,
locating the value by its being instanceof ZulipVersion), which
doesn't depend on knowing the value's location in the Redux state

I think, when we last spoke about this in the office, you were still weighing the pros and cons of each, and didn't favor one over the other. If it's helpful, I can put together another PR with the other approach, just to see what that might look like.

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.

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 with JSON.stringify 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, convert the ZulipVersion instance into the raw version string with .raw(), and on exit, call the ZulipVersion constructor on that raw version string. We use redux-persist's `createTransform`. We locate a value to apply this transformation to with an "outside-in" approach, in particular, by the value's extrinsic property of where it is in the Redux store. That is, we know that it's at the `zulipVersion` key on all Account objects in state.accounts. Instead, we might have used an "inside-out" approach to locate a value by its *intrinsic* property of being a ZulipVersion instance. (We don't insist on using a single approach for all future transforms, but it's useful to compare the two because it's possible that some kinds of transformations fit one way better than the other. See zulip#3950, "Use Map rather than object-as-Map in our Redux state", and zulip#3949, "Use good data structures in Redux state, to avoid quadratic work", for more things we'll need to transform.) The "outside-in" approach requires that we write and maintain code to establish the location of the values to transform. But the complexity of that code will be no more than the complexity of the reducer(s) in charge of the same part(s) of the state, so the burden isn't worrisome. Establishing the location is a two-step process: 1) In src/boot/store, tell the redux-persist transform, created with createTransform, what key you want to handle. (In this case, 'accounts'.) 2) In the reducer file corresponding to that key, write "save" and "load" functions that know where to find the value. (They also do the actual transformation.) It's helpful in step 2 to consolidate code into the reducer files, which already have code that's aware of the structure of the part of state being treated. (Further consolidation like this is zulip#3934.) If we had instead gone with the "inside-out" approach (i.e., locating the value by its being `instanceof ZulipVersion`), which doesn't depend on knowing the value's location in the Redux state, we might have used a handy feature of `remotedev-serialize` called "custom serialization functions" (see https://github.com/zalmoxisus/remotedev-serialize#passing-custom-serialization-functions). As noted in zulip#3949, at some point we'll probably use remotedev-serialize for the particular purpose of transforming Immutable.js data structures, so it may be convenient to piggy-back on that for this custom functionality. Greg explains that strategy 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` """ This would eliminate the need for custom code that tracks the location of the values to be transformed. Reasons we don't use this strategy here are that we don't expect the location of the ZulipVersion value to change, and that the remotedev-serialize strategy always requires writing a migration; old data has never been persisted with that __serializedType__ property. Fixes: zulip#3951

chrisbobbe · 2020-04-16T18:00:42Z

OK, I've just resolved those merge conflicts. In that first commit, "redux types: Make zulipVersion string | null.", I made the necessary changes to the getServerVersion selector, but I didn't change the empty value returned by getServerVersion from void to null, which is probably a change we want to make, for consistency. Should I go ahead and do that, too?

gnprice · 2020-04-16T18:33:13Z

Great!

I made the necessary changes to the getServerVersion selector, but I didn't change the empty value returned by getServerVersion from void to null, which is probably a change we want to make, for consistency. Should I go ahead and do that, too?

Yeah, I agree -- let's go ahead and do that too. Could be either the same or a following commit.

gnprice · 2020-04-16T18:48:32Z

the "inside-out" approach (i.e.,
locating the value by its being instanceof ZulipVersion), which
doesn't depend on knowing the value's location in the Redux state

I think, when we last spoke about this in the office, you were still weighing the pros and cons of each, and didn't favor one over the other.

Mmm, yeah, thanks for refreshing my memory on the state of this.

If it's helpful, I can put together another PR with the other approach, just to see what that might look like.

Yeah, I think that alternate PR would be quite useful!

…ll`.

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 with JSON.stringify 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, convert the ZulipVersion instance into the raw version string with .raw(), and on exit, call the ZulipVersion constructor on that raw version string. We use redux-persist's `createTransform`. We locate a value to apply this transformation to with an "outside-in" approach, in particular, by the value's extrinsic property of where it is in the Redux store. That is, we know that it's at the `zulipVersion` key on all Account objects in state.accounts. Instead, we might have used an "inside-out" approach to locate a value by its *intrinsic* property of being a ZulipVersion instance. (We don't insist on using a single approach for all future transforms, but it's useful to compare the two because it's possible that some kinds of transformations fit one way better than the other. See zulip#3950, "Use Map rather than object-as-Map in our Redux state", and zulip#3949, "Use good data structures in Redux state, to avoid quadratic work", for more things we'll need to transform.) The "outside-in" approach requires that we write and maintain code to establish the location of the values to transform. But the complexity of that code will be no more than the complexity of the reducer(s) in charge of the same part(s) of the state, so the burden isn't worrisome. Establishing the location is a two-step process: 1) In src/boot/store, tell the redux-persist transform, created with createTransform, what key you want to handle. (In this case, 'accounts'.) 2) In the reducer file corresponding to that key, write "save" and "load" functions that know where to find the value. (They also do the actual transformation.) It's helpful in step 2 to consolidate code into the reducer files, which already have code that's aware of the structure of the part of state being treated. (Further consolidation like this is zulip#3934.) If we had instead gone with the "inside-out" approach (i.e., locating the value by its being `instanceof ZulipVersion`), which doesn't depend on knowing the value's location in the Redux state, we might have used a handy feature of `remotedev-serialize` called "custom serialization functions" (see https://github.com/zalmoxisus/remotedev-serialize#passing-custom-serialization-functions). As noted in zulip#3949, at some point we'll probably use remotedev-serialize for the particular purpose of transforming Immutable.js data structures, so it may be convenient to piggy-back on that for this custom functionality. Greg explains that strategy 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` """ This would eliminate the need for custom code that tracks the location of the values to be transformed. Reasons we don't use this strategy here are that we don't expect the location of the ZulipVersion value to change, and that the remotedev-serialize strategy always requires writing a migration; old data has never been persisted with that __serializedType__ property. Fixes: zulip#3951

chrisbobbe · 2020-04-16T22:27:15Z

Yeah, I agree -- let's go ahead and do that too. Could be either the same or a following commit.

Done.

…d of string. This commit should be read for its contrast with its counterpart, with the same title, at zulip#3952. I've tried to make this PR minimally different from that one, so a `git diff` or `git range-diff` might be informative. Here, we use the "inside-out" approach described there: > (i.e., locating the value by its being `instanceof ZulipVersion`), > which doesn't depend on knowing the value's location in the Redux > state As expected, it requires writing a (straightforward) migration. Unfortunately, I haven't gotten far enough to actually see this working, but I expect that our code in a working version won't need to be drastically different, so I'm submitting this for comparison anyhow. There appear to be some inaccuracies in the Flow libdef I cobbled together from the TypeScript with `flowgen` and `flow-typed create-stub`, but these shouldn't be difficult to fix. The current blocker, which blocks any use of ImmutableJS with Redux (an integration this PR depends on, as I noted at the bottom of zulip#3951 (comment)), is that we're stuck with an old version of `redux-persist`. Not until rt2zz/redux-persist@a94f29139, released in v6.0.0 (we're on ^4.10.2), do we get the ability to supply our own serialize/deserialize functions in the config passed to `persistStore`. The only other customization for replace/revive transforms that `redux-persist` allows is via a Transform created by `createTransform`, as we use in zulip#3952. But its contract requires that you supply functions that translate between serializable and non-serializable objects, without allowing us to take responsibility for the actual serialization. It would be a silly exercise, but it might be possible, to make it work with `createTransform` with an extra round-trip into JSAN [sic]. JSAN is what `remotedev-serialize` serializes into, and it claims to be more flexible than JSON. I believe zulip#2915 is our latest written knowledge about the drawbacks of `redux-persist` and why it ceases to be useful for us at version 5 and above, and it looks like we have a plan to vendor it.

chrisbobbe · 2020-04-17T00:51:01Z

Yeah, I think that alternate PR would be quite useful!

Opened as #4047! 🙂

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, with JSON.stringify, 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, convert the ZulipVersion instance into the raw version string with .raw() ("replace" it), and on exit, call the ZulipVersion constructor on 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 in this message. 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. 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), with custom replacer and reviver functions. 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 nothing has ever been stored in this `__serializedType__` format, we have to write a migration. It turns out to be very straightforward, and we expect it will be in the future. But we MUST remember to do so every time we use this approach in future. As mentioned above, this approach further clears the way for ImmutableJS. I've neglected to mention that the primary purpose of `remotedev-serialize` is to replace and revive ImmutableJS objects! (Note that we added `immutable` as a dependency in a recent commit.) So zulip#3949 and zulip#3950 will be much easier after this. The custom replacer/reviver functions we use here are a nice bit of flexibility provided on the side. Fixes: zulip#3951

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 in this message. 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 boilerplate every time. 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 way of storing the data. Don't confuse this with the *current* way of storing the "dead" form. 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 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 bit of flexibility provided on the side. (We added `immutable` as a dependency in a recent commit, since `remotedev-serialize` depends on it.) Fixes: zulip#3951 [1]: Actually, we use a wrapper around it, developed in a recent commit, to avoid a security hole. As long as we follow the pattern introduced in the two previous commits, using SerializeEscaped.immutable and its exported constant SERIALIZED_TYPE_FIELD_NAME, instead of Serialize.immutable (where Serialize is the default export of `remotedev-serialize`), we're fine.

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.

gnprice · 2020-05-05T19:34:51Z

#4047 is merged! Closing this one. Thanks for prototyping both designs.

chrisbobbe requested a review from gnprice March 4, 2020 22:11

chrisbobbe force-pushed the redux-persist-transform-zulip-version branch from 9db89ea to b68d3ab Compare March 4, 2020 22:12

chrisbobbe mentioned this pull request Mar 4, 2020

Keep the parsed ZulipVersion in Redux state, rather than raw string #3951

Closed

chrisbobbe changed the title ~~redux: Add transform to keep parsed ZulipVersion in Redux state.~~ [WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. Mar 4, 2020

rk-for-zulip reviewed Mar 5, 2020

View reviewed changes

chrisbobbe force-pushed the redux-persist-transform-zulip-version branch from b68d3ab to 97b70fe Compare March 7, 2020 00:21

chrisbobbe mentioned this pull request Apr 15, 2020

Store server's version number, include in Sentry events #3745

Closed

chrisbobbe force-pushed the redux-persist-transform-zulip-version branch from 97b70fe to 33e4f19 Compare April 16, 2020 17:56

Chris Bobbe added 3 commits April 16, 2020 15:16

accounts selectors: Make getServerVersion return `ZulipVersion | nu…

01273d4

…ll`.

accounts selectors [nfc]: Pure refactor of getServerVersion.

905f9d8

chrisbobbe force-pushed the redux-persist-transform-zulip-version branch from 33e4f19 to 905f9d8 Compare April 16, 2020 22:23

chrisbobbe mentioned this pull request Apr 17, 2020

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

Merged

gnprice closed this May 5, 2020

chrisbobbe deleted the redux-persist-transform-zulip-version branch November 5, 2021 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

[WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. #3952

[WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. #3952

chrisbobbe commented Mar 4, 2020 •

edited

Loading

chrisbobbe commented Mar 4, 2020

rk-for-zulip left a comment

rk-for-zulip Mar 5, 2020

chrisbobbe Mar 5, 2020

rk-for-zulip Mar 5, 2020

gnprice Mar 5, 2020

rk-for-zulip Mar 5, 2020

rk-for-zulip Mar 5, 2020

chrisbobbe commented Mar 5, 2020 •

edited

Loading

gnprice commented Mar 5, 2020

gnprice commented Mar 5, 2020

gnprice commented Apr 16, 2020

chrisbobbe commented Apr 16, 2020 •

edited

Loading

chrisbobbe commented Apr 16, 2020

gnprice commented Apr 16, 2020

gnprice commented Apr 16, 2020

chrisbobbe commented Apr 16, 2020

chrisbobbe commented Apr 17, 2020

gnprice commented May 5, 2020

[WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. #3952

[WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. #3952

Conversation

chrisbobbe commented Mar 4, 2020 • edited Loading

chrisbobbe commented Mar 4, 2020

rk-for-zulip left a comment

Choose a reason for hiding this comment

rk-for-zulip Mar 5, 2020

Choose a reason for hiding this comment

chrisbobbe Mar 5, 2020

Choose a reason for hiding this comment

rk-for-zulip Mar 5, 2020

Choose a reason for hiding this comment

gnprice Mar 5, 2020

Choose a reason for hiding this comment

rk-for-zulip Mar 5, 2020

Choose a reason for hiding this comment

rk-for-zulip Mar 5, 2020

Choose a reason for hiding this comment

chrisbobbe commented Mar 5, 2020 • edited Loading

gnprice commented Mar 5, 2020

gnprice commented Mar 5, 2020

gnprice commented Apr 16, 2020

chrisbobbe commented Apr 16, 2020 • edited Loading

chrisbobbe commented Apr 16, 2020

gnprice commented Apr 16, 2020

gnprice commented Apr 16, 2020

chrisbobbe commented Apr 16, 2020

chrisbobbe commented Apr 17, 2020

gnprice commented May 5, 2020

chrisbobbe commented Mar 4, 2020 •

edited

Loading

chrisbobbe commented Mar 5, 2020 •

edited

Loading

chrisbobbe commented Apr 16, 2020 •

edited

Loading