Framework: Don't bind selectors to state in connect

[mcsf]

• SiteSelector (SiteSelector: Remove selector state binding #14028)
• SitesDropdown (SitesDropdown: Don't bind selectors to state #14026)
• Sites (Sites: Don't bind selectors to state #14030)
• ESLint rule (Disallow functions in react-redux#connect() eslint-plugin-wpcalypso#38)

Antipattern

The following is a by-product on the work to remove SitesList, chiefly #13094. I'd like to have it tackled before the anti-pattern spreads.

As of this writing:

$ ag -Q 'bind( null, state'
components/site-selector/index.jsx
462:		getSite: getSite.bind( null, state ),

components/sites-dropdown/index.jsx
108:		getSite: getSite.bind( null, state ),

my-sites/sites/sites.jsx
106:			isSiteUpgradeable: isSiteUpgradeable.bind( null, state ),

Context

A few instrumental view-layer pieces that deal with sites were, under the SitesList paradigm, expecting from that library the ability to access any property of any site from the entire list of sites at any moment in Calypso's lifecycle (whether in the views, pre-, mid-, post-render, or elsewhere), and notably within component methods.

This clashes with our Redux-era way of:

• Stating data needs upfront in connect
• Being conservative wrt. the amount of data required
• Passing around (notably, parent-to-child passing) the least amount of data to signal what job is to be performed, e.g. passing siteId instead of site, leaving it to the descendant(s) to obtain the remainder via connect.

To avoid more blockage of #13094, the selector.bind( null, state ) anti-pattern was introduced as a very short-term fix. There are concrete technical reasons for why this is very bad. I was trying to find a longish comment I left on some PR recently where I detailed this a bit more, but the gist of it is that 1) there is the cost of instantiating a bound function on each mapState call, meaning every time global state changes, and on top of that 2) we lose the ability to shallowly compare old and new props on those connected components to avoid re-rendering, since the bound selector has a new reference each time it is instantiated.

Solution

Edit: Even though the rationale below has grounds, since a lot of SitesList-era behavior revolves around holding a lot of duplicate and/or derived state, we should take the opportunity to simplify logic in these components whenever possible. For instance:

• By making more parts only require and pass site IDs instead of site objects or site slugs, fewer pieces need access to Redux state.
• By delegating certain operations to action creators, thanks to Redux's thunk middleware, we can shift some logic that was performed in component methods away from the view. Example: navigateToSite in SiteSelector: Remove selector state binding #14028.

Feel free to ignore the following.

A common pattern in these remaining occurrences of selector binding is that the selector is required within the component because the data to be passed to the selector depends on component state. For instance, in SitesDropdown:

getSelectedSite() {
  return this.props.getSite( this.state.selectedSiteSlug );
}

A way to solve this is by having an intermediate layer that can pass this state into connect's mapState using a state-holding component:

ancestor
  ↓
empty component holding `selectedSiteSlug` and passing the following props:
    <SitesDropdown
      selectedSiteSlug={ this.state.selectedSiteSlug }
      setSelectedSiteSlug={ this.setSelectedSiteSlug }>
  ↓
Connect(SitesDropdown)
    computes and passes `selectedSite` to `SitesDropdown` proper
  ↓
SitesDropdown

A specialized version of this pattern was used in 5ad8cee so that SiteSelector could search sites based on user input.

Overthinking?

This lead me a few weeks ago to make a connectWithState interface that would solve this implicitly for cases like SitesDropdown. See gist:

connectWithState(
  ( state, selectedSiteSlug ) => ( {
    selectedSite: getSite( state, selectedSiteSlug ),
    primarySite: getSite( state, getPrimarySiteId( state ) ),
  } ),
  [ 'selectedSiteSlug' ]
)( SitesDropdown )
// the above passes `selectedSiteSlug` and `setSelectedSiteSlug` to `SitesDropdown`

[mcsf]

I've briefly discussed the aforementioned gist with @ockham, but looping in the other SitesList folks for thoughts: @gwwar @tyxla @mtias @ehg

[gwwar]

Thanks @mcsf

I'd like to have it tackled before the anti-pattern spreads.

I think it'd be worthwhile to add an eslint warning as well, with a link to why we shouldn't be using this pattern.

I've seen folks attempt to use this anti-pattern in PRs before.

[mcsf]

@gwwar totally agree, and it should also cover cases of explicit function declaration.

[tyxla]

Nice! I don't think I've seen that pattern being used too much, and I hope we won't be seeing it anymore too 😉

[mcsf]

→ Automattic/eslint-plugin-wpcalypso#38

[lamosty]

Can we close this? Looks like everything is done.