Data: withSelect: Subscribe only to reducer keys referenced in mapSelectToProps

[aduth]

This pull request seeks to explore a possible optimization to the implementation of withSelect, such that it becomes subscribed only to the stores on which it is selecting data.

Background:

When the editor loads, we have a number of state changes which occur across many distinct stores. Notably, there are a number of data resolutions which occur via the core-data package. While the stores themselves are lightweight, each of these changes forces every connected component to rerun its selectors. Particularly for large posts, this can severely bog down the initial load time of the editor. In my experience, there can be upwards of a 30-40 store changes after the first render of the editor. It can also have a runtime impact; for example, any change in, say, Yoast's own store would incur a mapSelectToProps to again be called for every single connected component in the page.

Implementation:

The changes here enable a developer, when subscribing to a registry, to provide a set of reducerKeys as a condition on which their listener is to be called. When a withSelect-connected component is mounted, it observes its initial call to mapSelectToProps to determine the reducer keys for which select is called. These are used as the set of reducerKeys in its own subscription. Thus, the mapSelectToProps for the component is only called when the store for those specific reducer keys has changed.

Implications:

Due to how the reducerKeys for the subscription is determined at the initial mounting of the connected component, this has a few implications:

• mapSelectToProps must call select consistently. This is already a best-practice anyways, but a mapSelectToProps must not have conditional logic under which it calls select with a different reducer key(s).
• Selectors cannot have side effects, including select calls to operate on other stores. If a selector derives its value including data from another store, changes to that other store will not cause the top-level selector call to be made.
  • Technically, there may be some options to explore here with having an observer similar to the one implemented for withSelect, but for the entire registry.
  • But... I think we must move away from this pattern, as calling wp.data.select from a selector should be considered an anti-pattern because it is not contextualized to a specific registry, and thus would produce wrong values when in the non-default registry (see also Reusable Blocks: Reimplement reusable block as embedded editor #7119). Separately, I think we should explore patterns for supporting selection across stores by making the registry (or, specifically its select function) available to a selector in some form (e.g. a bound this.registry, or a ( registry ) => ( state, a, b ) => {} higher-order function).

Results:

I was slightly underwhelmed by the result, but it is still a notable improvement. Using the content of the text from #11782 (comment) converted to blocks, I see a reduction of initial load (DOMContentLoaded) from ~22 to 16 seconds (roughly 27% improvement). As mentioned earlier, this should also have some noted improvement for general runtime usage, though not as much, since the majority of runtime behaviors impact the core/editor store, which is also the store subscribed to by the bulk of mounted components in the editor.

Other Ideas:

The fact that there are dozens of actions dispatched during the initialization of the editor could be argued as an issue in and of itself. I think there are some opportunities to limit this.

A few thoughts on this:

• We shouldn't want to discourage a module having its internal state management being implemented on the registry if it needs to make frequent changes to its own internal state. There is both a convenience and a consisting in adopting the data store pattern.
• We could explore letting the editor mount so that all data initialization is kicked off, and only on the next tick perform the parse on post content to populate the blocks in the editor. This way (a) something is shown to the user immediately and prior to the parse, and (b) the rendered blocks (which account for the majority of the connected components in a large post) aren't impacted by the initial data resolution because they're not mounted at the time they are initiated.
• For prepopulated data, we should consider whether there are options to nullify the resolver. The idea would be to "pre-resolve" state with the data provided from the server.
• As seen in the aforementioned Gist console log, core block registration is another major contributor to store changes during initialization. I think there ought to be a way to bulk-register blocks, either within the blocks module itself, or deferring the store dispatch to the initializeCoreBlocks function. However, since this occurs prior to the initial render, I'm not sure whether this actually contributes much to a delayed first load.

[jsnajdr]

mapSelectToProps must call select consistently.

Is this the same limitation that React Hooks have? I.e., all selects must always be called and must be called in the same order?

Also, what if we change the underlying implementation? Currently, every withSelect component maintains its own store subscription. But react-redux@6 moved on to use the new React context and there is only one subscription in the provider. It's now the React library's job to find all the consumers in the subtree under a provider and update them. Besides other things, it guarantees the right order of updates (parent first) without any complicated magic.

Does the new API introduced in this PR also make sense in an alternative implementation like the react-redux@6 one? And if yes, does it achieve any optimizations?

[aduth]

The primary optimization here is not in how a store subscription update is applied, but in whether it is applied; namely, avoid mapSelectToProps if the component isn't pulling data from the changed store. This is unlike both the previous implementation and react-redux (@6), which will call every consumer's mapStateToProps on every state change.

As to whether the optimization is viable to achieve if following a similar refactor (or substitution) toward a react-redux context approach? I could maybe imagine an implementation where state providers are store-specific, or somehow making the "updater callback" (consumer render) aware of the store which had been changed in deciding whether to update. It may be worth doing some deeper exploration of how this might be achieved before moving forward, to ensure it won't leave us in a hard spot.

But to be clear, it's not really so much an API change being proposed here as much as a formalization of what's always been considered a best practice (evidenced to an extent by lack of any existing components needing to be updated). I'm unfamiliar with the internal implementation of hooks; the consistency of calling is likely a similar need to be statically aware of which stores are being selected from. The order call on select within one's own mapSelectToProps is not important as it is in hooks.

[jsnajdr]

the consistency of calling is likely a similar need to be statically aware of which stores are being selected from. The order call on select within one's own mapSelectToProps is not important as it is in hooks.

After reading the patch for a second time now, I started to understand why the consistency of select calls matters: the first-ever call to mapSelectToProps will keep track of stores that were queried, and we'll subscribe only to these stores. If the second call queries additional store (e.g., one which was omitted due to an if condition on the first call), there will be no subscription for that new store.

mapSelectToProps must call select consistently. This is already a best-practice anyways

Is there really a best-practice like this? Never heard of it 🙂 In Classic Redux, there's no reason to worry about calling selectors consistently at all.

In the Calypso codebase, there is plenty of places where selectors are called conditionally:

const siteId = getSelectedSiteId( state );
const foo = siteId ? getSiteOption( state, siteId, 'foo' ) || 'default-foo';

const isJetpack = isJetpackSite( state, siteId );
const bar = isJetpack ? getJetpackSpecificBar( state, siteId ) : 'wpcom-bar';

const mapStateToProps = ( state, ownProps ) => {
  const siteId = ownProps.siteId || getSelectedSiteId( state );
}

The consistency requirement also adds constraints on how selectors calls can be memoized. What are the new rules for memoization? What kind of code continues to be reliable and what breaks?

All in all, I'm afraid the consistency requirement makes writing mapSelectToProps code a dangerous and error-prone adventure.

Would there be any performance (or other) penalty if we checked the getReducerKeys() result after every call and updated the subscriptions accordingly?

[aduth]

It's not consistency of selectors called, it's consistency of select called.

[jsnajdr]

It's not consistency of selectors called, it's consistency of select called.

When called like select( 'store' ).getFoo(), calling selectors and select is effectively the same thing.

What could work is requiring the select calls to be hoisted to a "preamble":

const mapSelectToProps = select => {
  const foo = select( 'foo' );
  const bar = select( 'bar' );
  const baz = select( 'baz' );

  // call any selector you want after this line
  foo.getFoo() ? bar.getBar() : baz.getBaz();
}

Automated Babel transform that does the hoisting is a possibility.

[gziolo]

What could work is requiring the select calls to be hoisted to a "preamble":

const mapSelectToProps = select => {
  const foo = select( 'foo' );
  const bar = select( 'bar' );
  const baz = select( 'baz' );

  // call any selector you want after this line
  foo.getFoo() ? bar.getBar() : baz.getBaz();
}

Automated Babel transform that does the hoisting is a possibility.

This is already de facto the way how most of the time withSelect is structured in Gutenberg. It might be also possible to introduce an Eslint rule to enforce that.

[aduth]

• Yes, it's an issue if a component would use a selector directly on the result of select in a condition.
• It's also true the "preamble" pattern is an (unenforced) recommendation for all usage.
• There's a third truth in being able to automate some of this with Babel, though the Babel transform can't be an assumed presence either.

There are certainly some caveats and potential "gotchas" with the proposal here. I'm not fully convinced it's the right way to go. In practice, it does have a significant impact on the runtime performance of the application, where we currently have what most would consider trivial internal actions of ancillary stores causing huge cascades of re-renders on all subscribed components.

It's partly why I also included the "Other ideas" section, since while this has a demonstrable impact, there's many other contributing factors which together have culminated in this being an issue. One of my worries is that one of the most convincing alternative "fixes" is that state like resolution status would be refactored to avoid using the data module abstraction, and that we'd generally become wary against leveraging the data module as a tool except in cases where we acknowledge that the "cost" is worthwhile. I'd rather it be a more positive choice to opt for, than framed in a negative perspective as having to be associated with some cost.

[jsnajdr]

A (maybe) crazy idea: with all the constraints we're placing on the calls to select, is it still a good idea to pass the function as argument to mapSelectToProps? The following API is equivalent to the "preamble with selects" option and doesn't give the programmer any chance to make a mistake:

const mapSelectToProps( [ 'core', 'core/rich-text' ], ( [ core, richText ] ) => {
  return {
    media: core.getMedia( ... ),
    format: richText.getFormatType( ... ),
  };
} );

[aduth]

@jsnajdr I like where this is going, particularly because it can be introduced as an opt-in enhancement.

Wondering if it's better or worse for the callback to just receive an object of selectors. My thinking is that it could allow for destructuring and avoid the uncertainty on the developer's part in choosing a name for the argument.

const mapSelectToProps( [ 'core', 'core/rich-text' ], ( selectors ) => {
  const { getMedia, getFormatType } = selectors;
  return {
    media: getMedia( ... ),
    format: getFormatType( ... ),
  };
} );

Edit: A potential issue with this could be if two namespaces share a common selector name.

[aduth]

A third option could be to keep everything exactly as it is today, with the namespaces as a separate optional array argument, as a "hint". It becomes similar then to what's already proposed in the pull request, except requiring a manual opt-in vs. trying to automate.

[jsnajdr]

Wondering if it's better or worse for the callback to just receive an object of selectors.

That introduces one global namespace for all selector functions, doesn't it? That's managable inside the core/* stores, but not for potential other users of @wordpress/data like Calypso, Jetpack or Woo. Or 3rd party block authors who can register their mycompany/plugin stores.

I think the following three variants of withSelect could work very well:

Subscribe to just one store:

withSelect( 'core', selectors => {
  return { media: selectors.getMedia( ... ) };
} );

Useful when you need just one store. Saves typing a lot of silly squared brackets. Optimized by subscribing to just one store.

Subscribe to multiple stores:

withSelect( [ 'core', 'core/rich-text' ], ( [ coreSelectors, richTextSelectors ] ) => {
  ...
} );

Slightly more powerful, subscribes to multiple stores and exposes their selectors. Still optimized by subscribing to a subset of stores.

Subscribe to everything:

withSelect( select => {
  return { media: select( 'core' ).getMedia( ... ) };
} );

The current implementation. The most generic one that just passes the select function. Doesn't optimize the subscribed stores -- subscribes to everything.

---

withSelect implementation can easily distinguish between the variants by looking at the type of the first argument.

There's a tradeoff between being generic/flexible and being optimized. The most constrained variant is the most optimized, the most flexible one is not optimized at all.

The three variants provide a nice product line of store-selection tools. Each model in the line is clearly differentiated from the others with regard to performance and flexibility.

[aduth]

Subscribe to just one store:

I like this one, and it could scale for selecting from multiple stores as part of a compose chain. I think the only issue would become the overhead of having additional wrapping components. This is something I think will work particularly well with React hooks, though. Something along the lines of:

const { getMedia } = useSelect( 'core' );
const { getFormatType } = useSelect( 'core/rich-text' );

[jsnajdr]

I like this one, and it could scale for selecting from multiple stores as part of a compose chain. I think the only issue would become the overhead of having additional wrapping components.

Yes, I agree that selecting from one store is a sufficient primitive. The mutliple-store, array version is just syntactic sugar that doesn't make any new things possible, just more convenient. Practice will tell whether composing multiple withSelect is a pleasant experience or not.

[aduth]

A combination of ideas already expressed occurred to me, which is that by having the additional argument hint as an opt-in enhancement, we could without much additional trouble combine this with a Babel transform which automatically injects the argument for core modules. The Babel transform isn't a requirement for using withSelect (or even the hint argument), but an optional enhancement which core modules take advantage of.

[aduth]

A combination of ideas already expressed occurred to me, which is that by having the additional argument hint as an opt-in enhancement, we could without much additional trouble combine this with a Babel transform which automatically injects the argument for core modules. The Babel transform isn't a requirement for using withSelect (or even the hint argument), but an optional enhancement which core modules take advantage of.

See: #13177

[aduth]

Going to close this one in favor of #13177