Store state.messages as an Immutable.Map.

[chrisbobbe]

After #4201, we're now using Immutable.Map for state.narrows. This continues that work by making state.messages use Immutable.Map, too; it's one of the priority areas Greg has described.

I haven't yet done any performance measurements; I'd be happy to take that up soon, unless we can already be confident enough that this won't cause any significant performance regressions.

One thing I'm concerned about (as mentioned in the main commit) is that, with the current types, Flow won't catch it if you accidentally pass Immutable.Map({ 1: 'foo' }) where a numeric-keyed Immutable.Map is expected. (That value will have string keys because { 1: 'foo' } is really { '1': 'foo' }.) See where we handle MESSAGE_FETCH_COMPLETE in messagesReducer for one example of getting this right, but where we wouldn't be alerted if we got it wrong.

[gnprice]

Thanks @chrisbobbe ! This looks great -- two comments below, both on followups.

When I rebase I get a few Flow errors, so I'll hold off merging; please merge at will after resolving those.

[gnprice]

This can be simplified to just the deleteAll call. (I'm maybe 95% confident that Immutable will indeed return the identical value if none of the keys are present -- but it'd be nice to make sure there's a messagesReducer test confirming that.) Simpler and a bit more efficient, too.

Best as a separate followup commit, as it's nice that this commit is such a straightforward translation.

[gnprice]

This pattern can be condensed like so (in a followup), using Immutable.map#update:

const eventReactionAdd = (state, action) =>
  state.update(
    action.message_id,
    oldMessage =>
      oldMessage && {
        ...oldMessage,
        reactions: oldMessage.reactions.concat({
          emoji_name: action.emoji_name,
          user_id: action.user_id,
          reaction_type: action.reaction_type,
          emoji_code: action.emoji_code,
        }),
      },
  );

A crucial feature here of update is that

if the updater function returns the same value it was called with, then no change will occur.

So if the message isn't present, the updater will be passed undefined, and just has to return undefined and the map will be returned unchanged.

I'm kind of ambivalent on this clarity-wise. It's a bit shorter, but does rely on a somewhat subtle fact about update's interface. But two things push me toward thinking that overall it's an improvement:

• It should be more efficient: the old code requires the key to be looked up in the data structure twice, and with this version Immutable can (and I expect it does) look it up just once.
• Because it no longer needs a local variable, it can be inlined conveniently into the switch in the reducer. With similar simplifications, all of these helpers can be inlined conveniently into the reducer. That's the style of most of our reducers, and I generally prefer it because it means one fewer hop to look up what the effect of an action is (or vice versa, to find in what situation some code found in one of these is called.)

Those and:

• If we start making use of this feature routinely, as we well might, then it becomes a familiar one 🙂 and not a puzzling subtlety.

[chrisbobbe]

Thanks for the review! Revision pushed. The Flow errors, it turns out, were because I hadn't added ownUserId to some messageFetchComplete actions in the messagesReducer tests (that bit of data was a recent addition).

[chrisbobbe]

You can see a draft of the followup I mention in the tip commit here.

[chrisbobbe]

I've just rebased across the changes in #4393, fixing a few minor conflicts.

[gnprice]

Thanks for the revision! Looks good -- merged.