Fix caching in "for narrow" selectors

[gnprice]

When we want to pass an argument to a selector, like props.narrow to getRenderedMessages, we currently do so in a way that totally defeats Reselect's caching. We should fix this.

See discussion in chat. In particular, on what to do instead:

The solution recommended in the Reselect docs is to push the inspection of props inside the selector:
https://github.com/reduxjs/reselect#accessing-react-props-in-selectors

I don't love that solution -- it feels awfully implicit to me. [...] I'd much rather stick to passing specifically the narrow down to the selectors, like we currently do.

I guess there's nothing in the Reselect code (AFAICT) that depends on that "props" argument actually being the props of any React component. So we could just say getRenderedMessages(state, props.narrow), and have all the ...ForNarrow selectors take a second argument of type Narrow, and that may just all work great.

(We'd be treating that Narrow as what the Reselect docs expect to be "props".)

So instead of defining selectors so that their call sites look like getRenderedMessages(props.narrow)(state), we should define them so they're called like getRenderedMessages(state, props.narrow).

I don't think this is currently a top priority, though.

[gnprice]

An update on how to approach this, something I learned from studying reselect and react-redux after reviewing #2450: in many of these selectors, we really wouldn't get a meaningful benefit from createSelector even if we used it correctly! For example:

export const getFetchingForActiveNarrow = (narrow: Narrow) =>
  createSelector(getFetching, fetching => fetching[JSON.stringify(narrow)] || NULL_FETCHING);

This should just be

export const getFetchingForActiveNarrow = (state: GlobalState, narrow: Narrow) =>
  getFetching(state)[JSON.stringify(narrow)] || NULL_FETCHING;

(Also it should be renamed by s/Active//.) Even if we used createSelector correctly, what the resulting selector would do when called is basically

• (We're here because either state or the component's props has changed.)
• Check if either state or narrow has changed.
• If so, recompute fetching; else we're done.
• Check if either fetching or narrow has changed.
• If so, recompute the final result.

Whereas the simpler thing, when called, does

• (We're here because either state or the component's props has changed.)
• Compute fetching.
• Compute the final result.

This will always recompute fetching, but that should either be cheap (like in this example) or have its own caching... and it will always recompute the final result. But on the other hand there's some small cost to those "check if changed" lines -- and most importantly, the simpler thing is much simpler to reason about and fully understand.

So in general, whenever the function that computes the final result -- i.e. that last argument to createSelector -- is doing very little work, we're better off dropping the createSelector. In particular if it's just a property access or two and some boolean logic, let's cut createSelector -- my guess is that it's actually spending more time going through the abstractions to check the cache than we would spend just computing the result directly.

(It's possible this is actually a bad example because there's that JSON.stringify; the caching can avoid that if the state changed only outside of fetching. But that mainly illustrates that we ought to refactor our representation of narrows to something cleaner.)

---

The original example getRenderedMessages, though, is one where there can be a ton of work happening and so we definitely want the caching.

So are a bunch of the selectors in narrowsSelectors, where we go do some work on every message in the narrow.

[gnprice]

I've just pushed a handful of commits that make a start on this. See discussion in cc9628e and 2d23ea7 and a bit in 3182501, and additional examples in 61c84e3^..3d9baba.

Plenty more to do! I count 20 more selectors this applies to, using this search command:

$ perl -0ne '
          print "$ARGV: $1\n" while (/^export const (\w+) = [^\n]*=>\s*createSelector/msg)        
        ' src/**/*.js
src/chat/fetchingSelectors.js: getFetchingForNarrow
src/chat/fetchingSelectors.js: getIsFetching
src/chat/narrowsSelectors.js: getStreamInNarrow
src/chat/narrowsSelectors.js: getIfNoMessages
src/chat/narrowsSelectors.js: getShowMessagePlaceholders
src/chat/narrowsSelectors.js: isNarrowValid
src/drafts/draftsSelectors.js: getDraftForNarrow
src/message/messageSelectors.js: getRenderedMessages
src/message/messageSelectors.js: getAnchorForNarrow
src/subscriptions/subscriptionSelectors.js: getIsActiveStreamSubscribed
src/subscriptions/subscriptionSelectors.js: getStreamFromId
src/subscriptions/subscriptionSelectors.js: getSubscriptionFromId
src/subscriptions/subscriptionSelectors.js: getIsActiveStreamAnnouncementOnly
src/title/titleSelectors.js: getTitleBackgroundColor
src/topics/topicSelectors.js: getTopicsForNarrow
src/topics/topicSelectors.js: getTopicsForStream
src/topics/topicSelectors.js: getLastMessageTopic
src/typing/typingSelectors.js: getCurrentTypingUsers
src/unread/unreadSelectors.js: getUnreadCountForNarrow
src/users/userSelectors.js: getAccountDetailsUserFromEmail

[gnprice]

Plenty more to do! I count 20 more selectors this applies to, using this search command:

Or, if you find the search UI in VS Code convenient, I just learned that it supports multiline searches!

Hit Ctrl+Shift+F (for "Search: Find in Files"), then Alt+R (to toggle "Use Regular Expressions"; equivalently hit the .* icon in the search box); then paste the query =>\s*createSelector\n?. (The silly \n? is there because, as documented at the link above, this VS Code feature only activates when an explicit \n is present in the query pattern.)

You should get, as of 5764364, 20 results in 10 files -- same as listed in my previous comment.