Use Immutable for state.flags.

[chrisbobbe]

This would be another instance of #3949 and #3950.

Once something like #4201 lands, we'll have a roadmap for doing this. Done!

In addition to the performance benefit, this would be a big improvement to code quality. The FlagsState type is the following:

export type FlagsState = {|
  read: { [messageId: number]: boolean },
  starred: { [messageId: number]: boolean },
  collapsed: { [messageId: number]: boolean },
  mentioned: { [messageId: number]: boolean },
  wildcard_mentioned: { [messageId: number]: boolean },
  summarize_in_home: { [messageId: number]: boolean },
  summarize_in_stream: { [messageId: number]: boolean },
  force_expand: { [messageId: number]: boolean },
  force_collapse: { [messageId: number]: boolean },
  has_alert_word: { [messageId: number]: boolean },
  historical: { [messageId: number]: boolean },
  is_me_message: { [messageId: number]: boolean },
|};

I'm not sure state.flags itself should be made an Immutable.Map because it has a certain set of named and non-optional properties (see FlagsState above), but the values at those properties certainly look like good candidates for Immutable.Maps. I'm not sure what we should land on for state.flags, but I do think it should be something from Immutable if we can.

In addition to the performance benefit, this will also be good for code quality and correctness. Currently, we have two instances of const newState = {}; in the flags reducer; that contradicts the FlagsState type (its properties are not optional). Flow v0.113, which we get with RN v0.62 (#3782), will start to have an issue with this, indirectly. An obvious solution would be to initialize newState to initialState instead of {}; the properties we care about updating will clobber the ones on initialState, and it's much clearer that we won't end up with an incomplete state stored. But the implementation is such that initialState itself—defined once, at the top of flagsReducer—would get mutated. Which is no good. So I think it would be great to try to rework the logic with immutable data structures.

[gnprice]

Sounds like a good plan to me!

I'm not sure state.flags itself should be made an Immutable.Map because it has a certain set of named and non-optional properties (see FlagsState above), but the values at those properties certainly look like good candidates for Immutable.Maps. I'm not sure what we should land on for state.flags, but I do think it should be something from Immutable if we can.

I think this is likely a good use case for Immutable.Record:
https://immutable-js.github.io/immutable-js/docs/#/Record
But I haven't actually played with that data structure yet, or read its whole docs closely.

[chrisbobbe]

I think this is likely a good use case for Immutable.Record:
https://immutable-js.github.io/immutable-js/docs/#/Record
But I haven't actually played with that data structure yet, or read its whole docs closely.

Hmm, maybe! I came across that while putting this issue together, but I kind of got stuck on the name (and then still as I read a bit of its doc)—it sounds like a struct-like thing you'd reach for when you want to hold information about a single entity. FlagsState is meant to hold information about lots of entities. I'm not sure what any negative consequences might be if we used it, though.