Sites: Use Redux state preference for recent sites

[aduth]

Related: #7323, #7413

This pull request is a continuation of #7323, migrating the <SiteSelector /> component toward using Redux-based global state for recent site preferences. It completes the migration, removing recent site logic from the sites-list module.

Toward #7413, it resolves a warning logged in development environments related to the preferences prop passed to the rendered div by the legacy <PreferencesData /> component.

Testing instructions:

There should be no changes in behavior from the master branch. Notably, confirm that your recent sites are shown in the correct order, and that selecting a new site updates this order and is persisted between page sessions.

Caveats / Open Questions:

• It's not really specific to this pull request, but I noticed there can be a race condition when saving the recent sites preference while a request is in progress for preferences. If the preference fetch request completes after the update request, the preference value can revert back to its previous value. This is noticeable when loading a page for a site which is not currently the most recently selected site.
• The <SiteSelector /> doesn't render a <QueryPreferences /> component of its own, instead depending on the one rendered in the top-level <Layout /> component. This is to avoid excessive requests when toggling the visibility of the <SiteSelector /> component. It could be argued that this is desirable to keep the preferences data fresh (which would probably resolve the issue in the first noted caveat), but with <QueryPreferences /> guaranteed to be rendered on every screen by <Layout />, it should be safe to rely on this as the source of preference data.

/cc @mtias @gwwar

Test live: https://calypso.live/?branch=update/site-selector-preferences

[gwwar]

The race condition sounds similar to the one we had for settings: #1167

In this case, what if we added some state when we switch sites that lets the preferences reducer know not to override the recent site preference, but lets it update everything else.

[gwwar]

Let's remember to update this later when we have a bit more ported. We should be able to avoid reducing over all sites.

[aduth]

Let's remember to update this later when we have a bit more ported. We should be able to avoid reducing over all sites.

Yeah, I had the same thought while refactoring. If we had sites already indexed by ID (which is the case for Redux where we can use getSite), we'd be able to map over the recentSites array instead of the other way around. As-is, this way is still much more performant than previously, where for each recent site we'd filter over the entire sites array.

[gwwar]

where for each recent site we'd filter over the entire sites array.

😄 oh that's a fun one

[gwwar]

👍 Didn't find any regressions. Let's 🚢

[aduth]

Some last minute testing revealed a few oddities and opportunities for improvement which I've addressed in the latest batch of commits:

• Using Array#splice didn't work so well when the order of recent sites wasn't consistent with the order of the sites array, so I went the route of mapping over recent sites onto a sites object keyed by ID (598a28c)
• Many of the my-sites controllers were not rendered with Redux context, which had broken the /sites route when combined with the changes here. All sites controllers now render with Redux context (3c9d565, cc @sirbrillig re p4TIVU-4gd-p2)
• I followed the logic of connect and found that when a connected component is marked as unpure, it will always render on state dispatch, even if the state selector will not result in a changed prop. This felt quite inefficient for what is a fairly omni-present component, so dealt with the one potentially troubling prop (sites) and removed the { pure: false } option (cd25a32)

[gwwar]

ping me after you rebase, and I can take the branch for another spin

[aduth]

Thanks for the heads-up on conflicts. Guessing #7315 will hit quite a few open PRs.

Pushed a rebase.

[gwwar]

Good call with removing the pure option, it feels much faster without it.

👍 Retested this on the main my-sites routes, and verified that the behavior looks correct for recent sites.

[aduth]

I was pretty wary about the Redux render context issues, so went through and tried to find and update as many ReactDOM.render not already rendering with context and updated them as part of 31f18f5, even though not directly related to the intended changes here.

@sirbrillig do the changes in 31f18f5 make sense to you?

[sirbrillig]

@aduth Yeah, it looks great! (With one exception - commented inline - that may be fine but I don't fully understand.)

[aduth]

FYI this was reverted in #7556 and there is in-progress refactoring to the preferences state in #7566 which will remedy one of the two problems leading to its reverting (the other being not preserving the 12 site minimum for recent sites).