Gutenberg: Data: Implement useSelect hooks corollary to withSelect

[aduth]

The introduction of hooks in React 16.8 provides a simple pattern for stateful behaviors, enabling encapsulation of lifecycle events and side effects. This aligns quite well to the existing pattern of data-bound components, currently expressed using the higher-order components withSelect and withDispatch.

Advantages:

There are many purported advantages to a hook-based alternative, chief amongst them being that higher-order components can be confusing and verbose to use in a component. Furthermore, the introduction of an optional hooks paradigm allows some reevaluation of existing patterns, such as what had been discussed in #12877 and #13177 with store-specific subscriptions.

Deterrents:

Hooks are a new concept which requires some amount of onboarding to become comfortable working with. That there is already a pattern of higher-order components in the codebase should demand some intentionality to the implementation of hooks generally, since we risk confusion stemming from inconsistency and fragmentation.

It should be possible to dismiss these worries if we can grant:

• It is assumed hooks are easier to become familiar and to work with than higher-order components (from the perspective of the consuming component).
• There is significant overlap between the goals of hooks and most higher-order components such that there is always a graceful transition path. We were also considerate enough to have established a naming convention (with*) that adapts quite well to a hooks equivalent (use*).
  
  
  
  • To assure a consistent experience one way or the other, we should recommend that in the adoption of the hooks pattern, we provide hooks alternatives to _all higher-order components_ currently available in packages offerings. Depending on a decision to consider higher-order components as legacy, this should be bidirectional (i.e. new hooks offered with an equivalent higher-order component).
  
  

Prior art:

• react-redux:
  
  
  
  • Discussions: https://github.com/reduxjs/react-redux/issues/1063, https://github.com/reduxjs/react-redux/issues/1177
  
  
  • Alpha documentation: https://react-redux.js.org/next/api/hooks
  
  
  • The current proposals for hooks usage in react-redux diverge from its own API of connect in offering useSelector and useActions hooks. This paradigm overlaps quite well with the withSelect and withDispatch higher-order components of @wordpress/data, as further reinforcement to the fact that useSelect and useDispatch are both possible and ideal.
  
  
  • The specifics of these APIs will likely be different, since @wordpress/data operates at a higher-level of abstraction than react-redux (operating through action-dispatchers and selectors rather than direct access of store.dispatch and state).
  
  

Proposal:

@wordpress/data will implement and make available a new useSelect and useDispatch pair of hooks. These will largely overlap with withSelect and withDispatch higher-order components in providing access to a store's selectors / action dispatchers, including necessary subscription behaviors.

useSelect will accept one, the other, or both of a storeName and mapSelectToReturnValue function. Likewise, useDispatch will accept storeName and/or mapSelectToReturnValue. The intention here is for interoperability with withSelect and withDispatch, which will likewise be updated to add support for storeName or optionally omit mapSelectToProps / mapDispatchToProps. For most usage, it would be assumed that a developer would pass the storeName argument and receive an object of selectors / action-dispatchers.

_Edit (2019-05-13):_ Per subsequent discussion in this issue, this proposal is _not_ representative of the advised implementation. See https://github.com/WordPress/gutenberg/issues/15473#issuecomment-489974224 and https://github.com/WordPress/gutenberg/issues/15473#issuecomment-490057826 for more detail. The issue will be revised in the future with the alternative recommended implementation.

function useSelect( storeName: string ): Object;
function useSelect( storeName: string, mapSelectorsToReturnValue: Function ): any;
function useSelect( mapSelectToReturnValue: Function ): any;
function useDispatch( storeName: string ): Object;
function useDispatch( storeName: string, mapDispatchersToReturnValue: Function ): any;
function useDispatch( mapDispatchToReturnValue: Function ): any;

Example of an adapted PostStatus component:

function PostStatus() {
    const { isEditorPanelOpened } = useSelect( 'core/edit-post' );
    const { toggleEditorPanelOpened } = useDispatch( 'core/edit-post' );

    return (
        <PanelBody 
            title={ __( 'Status & Visibility' ) }
            opened={ isEditorPanelOpened( 'post-status' ) }
            onToggle={ () => toggleEditorPanelOpened( 'post-status' ) }
            className="edit-post-post-status"
        >
            { /* ... */ }
        </PanelBody>
    );
}

_Considerations:_

• Since useSelect returns selectors and not the results of selector values, there's a risk of one of (a) verbose logic in a separate variable assignment for the result of the selector calls or (b) not assigning the selector call result to a variable and instead calling the selector multiple times as necessary (a potential performance concern).
• For better or worse, we lose the abstracted distinction of what's commonly referred to as Presentational and Container components, where previously PostStatus was a sort of visual component agnostic to the source of its isOpened and onTogglePanel props.

_Compatibility:_

As noted above, the argument signature should be made identical between withSelect / useSelect and withDispatch / useDispatch. Specifically:

• A component could be wrapped as withSelect( 'core/editor' ) and receive as props all available selectors for a given store.
• A component could be wrapped as withSelect( 'core/editor', mapSelectorsToProps ), and mapSelectorsToProps would be called with an object of selectors for the given store, expected to return a mapped object of props to provide with the component

The intention here is two-fold:

• Optimize for the more direct access of selectors offered by the useSelect( storeName ) form
• Enable opt-in performance enhancements made possible by known store dependencies of a component (limit component subscriber to be called only upon changes to relevant stores).

_Unknowns:_

• The discussions in prior art of React Redux are _extensive_, and may include some useful guidance or potential implementation barriers
• There are additional hurdles to the proposed performance optimization of limited subscribers, and it may not be worth investing in this direction without an understanding of whether those can be overcome.
  
  
  
  • Specifically: Transitive dependencies of stores, discussed at https://github.com/WordPress/gutenberg/pull/13177#issuecomment-465277070
  
  
• Given the considerations above, whether there are alternative interfaces which could more gracefully offer access to values produced by a selector, while retaining consistency with useDispatch and compatibility with withSelect.

[nerrad]

A couple of initial thoughts (I'm sure more will come as I have time to ponder this more):

• I think there's good value in supporting both the container model (withSelect etc) and introducing the new hooks model ( useSelect etc). Both have strengths in various scenarios. React also seems committed to supporting both class components and function components for a while too so deprecation isn't anywhere on the immediate horizon.
• I wonder if there'd be value in having the hocs (withSelect etc) utilize the new hooks under the hood? That would reduce some of the maintenance overhead if it's possible.

[aduth]

• I wonder if there'd be value in having the hocs (withSelect etc) utilize the new hooks under the hood? That would reduce some of the maintenance overhead if it's possible.

Yeah, I think that would be ideal, and true of any higher-order component we choose to adapt to a hook (and vice-versa).

[youknowriad]

Some thoughts:

const { isEditorPanelOpened } = useSelect( 'core/edit-post' );

I'm not sure we should offer this possibility as this means the components rerenders everytime the store edit-post changes while right now we do a shallow comparison for the selectors result first.

function useSelect( storeName: string, mapSelectorsToReturnValue: Function ): any;

Right now, withSelect doesn't offer a way to choose a given storeName and if it were to be added it would be added as a second argument I asssume.

• Should we add this here from the start? What does it solve? Performance (I have doubts)?
• Should we move it to the second argument?

function useSelect( mapSelectToReturnValue: Function ): any;

I think a dependencies array as a last argument makes sense, we won't have to handle the dependency management ourselves since it will most likely based on useEffect dependencies or something else built-in React. This could be a good performance win in some cases where we'll only run the selectors that depend on given props and not all the props.

Implementation

• Implementation wise, there's a single problem I'm wondering about, how do we subscribe to stores in the correct order (the parent component before the child one). Since this will most likely based on effects useEffect will use the opposite in order by default (child then parent) which could lead to bugs (rerendering a removed block...)

[aduth]

const { isEditorPanelOpened } = useSelect( 'core/edit-post' );

I'm not sure we should offer this possibility as this means the components rerenders everytime the store edit-post changes while right now we do a shallow comparison for the selectors result first.

Ah, that's a very critically important point to raise. In that case, I suppose we must have the logic occur in the mapping function, rendering (via updated state useState()[ 1 ]) only if the produced values have changed.

function useSelect( storeName: string, mapSelectorsToReturnValue: Function ): any;

Right now, withSelect doesn't offer a way to choose a given storeName and if it were to be added it would be added as a second argument I asssume.

While not explicit, the idea was that what was proposed for useSelect would have identical arguments signature options in the withSelect version as well:

function useSelect( storeName: string ): Object;
function withSelect( storeName: string ): Function;
function useSelect( storeName: string, mapSelectorsToReturnValue: Function ): any;
function withSelect( storeName: string, mapSelectorsToProps: Function ): Function;
function useSelect( mapSelectToReturnValue: Function ): any;
function withSelect( mapSelectToProps: Function ): Function;

Given what you've noted about the form with storeName as the only argument, it may be wise to continue to emphasize only what exists today with mapSelectorsToProps. At that point, I'm not sure it makes sense to include the store name as an argument for any variation.

function useSelect( mapSelectToReturnValue: Function ): any;

I think a dependencies array as a last argument makes sense, we won't have to handle the dependency management ourselves since it will most likely based on useEffect dependencies or something else built-in React.

What values are you proposing as members of the array? Are you thinking as an array of store names, to limit the callback to be triggered only on updates to those stores?

[youknowriad]

What values are you proposing as members of the array? Are you thinking as an array of store names, to limit the callback to be triggered only on updates to those stores?

No, not really I'm thinking of props/other component variables used in the mapSelectToReturnValue function.

[nerrad]

I think a dependencies array as a last argument makes sense, we won't have to handle the dependency management ourselves since it will most likely based on useEffect dependencies or something else built-in React.

I think the dependency array should work similarly to useEffect dependency rules (as that's mostly where the dependencies would be implemented).

The way I was think providing a storeName (or names) arguments could work - eg something with this signature:

function useSelect( storeName: array )

...is that providing storeNames implies that you not only want the selectors from those stores but also only want to rerender on changes to those specific store states. I realize that we'd need some work done so that listeners could be subscribed to a specific store.

[nerrad]

I realize that we'd need some work done so that listeners could be subscribed to a specific store.

I'm going to do an experimental pull because I think this should be relatively straightforward to do.

Edit: Forgot, and was reminded that @aduth already did some experimentation on this so I closed the pull I created in favor of his (#13177).

[epiqueras]

Hi guys, I see a few issues with the API proposed and would like to propose a different one.

Proposal:

/**
 * This would re-render every time the store receives an update.
 * I don't see this being used over the other signatures.
 * It could also be composed from other hooks.
 */
function useSelect( storeName: string ): Object;

/**
 * I don't see this being used over the other signatures.
 * It could also be composed from other hooks.
 */
function useSelect( storeName: string, mapSelectorsToReturnValue: Function ): any;

/**
 * This works, although it is missing an optional dependency array.
 * I would also add an optional array of store names to subscribe to,
 * now that `subscribe` supports it.
 */
function useSelect( mapSelectToReturnValue: Function ): any;

/**
 * This works, but we can drop the store name and just return the function.
 */
function useDispatch( storeName: string ): Object;

/**
 * Not necessary as action creators don't change like state does.
 */
function useDispatch( storeName: string, mapDispatchersToReturnValue: Function ): any;

/**
 * Not necessary as action creators don't change like state does.
 */
function useDispatch( mapDispatchToReturnValue: Function ): any;

Revision:

/**
 * Low-level hook that provides direct access to the registry.
 * Used in the implementation of the other 2 hooks.
 */
function useRegistry(): Object;

/**
 * Simple, 1-to-1 mapping with `withSelect`.
 * Only re-renders when the output of `mapSelectToReturnValue` is different.
 * `storeNames` lets you optionally subscribe to specific stores to avoid
 * unnecessarily running `mapSelectToReturnValue`.
 * `dependencies` lets you optionally memoize `mapSelectToReturnValue`
 * until one of the values provided changes. 
 */
function useSelect(
  mapSelectToReturnValue: Function, storeNames: String [], dependencies: any []
): any;

/**
 * Returns the `dispatch` function.
 * Action creators shouldn't change, so this is sufficient.
 */
function useDispatch(): Function;

I think this is as simple as you can get while still covering every use case and performance concern. Let me know your thoughts.

As for this point:

Implementation

Implementation wise, there's a single problem I'm wondering about, how do we subscribe to stores in the correct order (the parent component before the child one). Since this will most likely based on effects useEffect will use the opposite in order by default (child then parent) which could lead to bugs (rerendering a removed block...)

This can be avoided with a bit of defensive coding, see https://react-redux.js.org/next/api/hooks#stale-props-and-zombie-children

Also, note that the current implementation of withSelect also has this issue. React Redux uses a nested hierarchy of subscriptions to get around it. It creates a new context provider at the level of every connected component and overrides the subscription for all its children. This way, their subscription implementation can always enforce the order of updates. I also think rebuilding this on top of React Redux should be considered. Things get complex very fast when you start to factor in all the edge cases. withSelect and withDispatch could be composed from connect and their provider would integrate fairly easily. Am I missing the reason why this was not done this way?

[nerrad]

@epiqueras thanks for the feedback have you seen #15512 by any chance?

[epiqueras]

Hi @nerrad,

I think it makes more sense to have a mapping function rather than a hook per selector. Hooks can't be called conditionally so that will break down whenever you want to call a selector based on/with the results of another selector.

Also, that implementation re-renders every time the store updates, not just when the result of the selector changes. That is bad for performance and since selectors can trigger state updates, it can run into an infinite loop. It also doesn't handle the case when arguments change but the store has not updated.

[youknowriad]

```/**

• Simple, 1-to-1 mapping with withSelect.
• Only re-renders when the output of mapSelectToReturnValue is different.
• storeNames lets you optionally subscribe to specific stores to avoid
• unnecessarily running mapSelectToReturnValue.
• dependencies lets you optionally memoize mapSelectToReturnValue
• until one of the values provided changes.
  */
  function useSelect(
  mapSelectToReturnValue: Function, storeNames: String [], dependencies: any []
  ): any;```

I think I agree with this proposal with just one remark. I see the dependencies as an argument that is going to be more commonly used than a stores argument. I'd personally switch these two arguments and I don't think at the moment there's huge value in having the stores argument. Maybe we can just start with:

useSelect( mapSelectToReturnValue, dependencies )

re #15512

@nerrad I don't think hooks returning hooks is a good idea (do we have examples in the React community?). It feels too adventurous for me given the strictness of the hooks rules. In such a workd the inner hook implementation could change breaking the hooks rule?

[nerrad]

The comments on #15512 are along the lines of what I was already thinking (see my notes in the ticket). So it's great to get that confirmation. I mostly was coming at it from the perspective of exposing the selector hooks for simpler implementations where just a smaller number of selectors are needed in an implementation. As noted in the pull I do not think this should be the _only_ usecase for useSelect.

In such a workd the inner hook implementation could change breaking the hooks rule?

I'm not sure the current iteration would hit that because the hooks aren't actually invoked in the factory. So rules of hooks can still be followed. It's more just exposing each selector individually as their own hooks.

I also like the proposal currently in here. I agree with Riad having a stores argument is a bit unnecessary right now because we don't support store specific subscriptions yet internally. So it would make sense in that case to have dependencies as the second argument (leaving the option for later iterations to add a store argument once we support store specific subscriptions).

[epiqueras]

@nerrad
Yeah, that pattern does not break hook rules, but there are simpler ways to go about it with function composition, and that is what the best practices of hooks suggest.

@youknowriad
If we don't support store specific subscriptions, then yeah, we can drop that parameter. But, if we did, it would be used more than dependencies.

• dependencies is used to memoize mapSelectToReturnValue instead of using the latest one on every render and state update. This is only useful if mapSelectToReturnValue is itself a memoized function, because you don't want to use a new function, with a new cache, on every render.
• storeNames is used so your potentially expensive mapSelectToReturnValue is not called when it doesn't need to. Imagine you have a store that gets updated very frequently and unnecessarily triggers an expensive mapSelectToReturnValue for another store.

The former will seldom be used for our use cases. The latter will be common, although, most of the time, neither will be used. Also, it's semantic for the hooks dependencies array to be the last parameter in all hooks.

[aduth]

@epiqueras Store-specific subscriptions are certainly an optimization worth exploring, and have been explored in the past as well:

• #13177
• #12877
• #15483

The primary issue (also explained in the discussion of these pull requests) is in how to properly track dependencies _between_ stores. The discussion resulting from #13177 (and specifically https://github.com/WordPress/gutenberg/pull/13177#issuecomment-465277070) have some ideas for how this might be implemented in the future.

_However_, I don't know that it'd ever be something the developer themselves would express, rather than being programmatically generated during the build step. Regardless whether that comes to fruition, I don't see it necessarily being incompatible with the proposed signature (though it is unclear whether it would become a separate, third argument, or somehow combined into the single dependencies argument, or as an enhancer to invoke the function on store changes).

[epiqueras]

@aduth
Interesting, I think that the recursive dependency resolution outlined in your comment would work. And, yeah, a similar Babel transform can be used here.

Yeah, it's not incompatible. I was just making a case for storeNames to come first, to counter this comment.

I think I agree with this proposal with just one remark. I see the dependencies as an argument that is going to be more commonly used than a stores argument.

You can't combine them because the items are used as deps for a useMemo hook so mapSelectToReturnValue would be memoized forever. You could filter out store names from the list, but that's kind of messy.

React Redux actually opted to let users memoize the selector themselves before passing it down, now that I think more about it, that makes more sense. So let's just drop the dependencies parameter altogether.

Here, I made a prototype, let me know what you think:

https://gist.github.com/epiqueras/7eae39ba6b903286cf17a4907902a630

[nerrad]

Just an update, I've currently got a branch I'm working on locally that uses much of what @epiqueras provided in his gist (thanks!) and I'm experimenting with implementing the new hook in withSelect (which I think makes the result much more straightforward).

[epiqueras]

@nerrad Wouldn't it be easier to build these bindings on top of React Redux and get everything for free?

[epiqueras]

I see two main flaws with the current model that are not easy to solve:

• It's not easy to implement store specific subscriptions.
• There is no subscription hierarchy in the tree. This causes the dreaded zombie children and stale props bugs when a component that uses withSelect has a child somewhere in its subtree that also uses withSelect.

Using React Redux would solve these two issues and allow us to benefit from all the new optimizations and features that their maintainers are working on, including React Hooks support.

I think the registry can use React Redux's createProvider every time a store is registered, and then withSelect can call React Redux's connect for every referenced store. We'd also need a top level RegistryProvider that nests all of the stores' Redux Providers, but this is required to keep a subscription hierarchy regardless.

[youknowriad]

@epiqueras We actually started the data module by relying on react-redux and changed that down the road.

One of the main reasons is the fact that withDispacth uses profixied props to avoid regenerating the functions once the props change. This means there's an assumption that the returned prop names don't change conditionally (but that assumption is I think something almost "granted" for withDispatch). This proved to be a big win in terms of performance.

The second performance gain we operated is the "Async" mode. AFAIK, react-redux don't have a support for that kind of subscriptions and I'm not sure it will be easy to implement above react-redux without touching its core.

[epiqueras]

@youknowriad Thanks for the background info!

One of the main reasons is the fact that withDispacth uses profixied props to avoid regenerating the functions once the props change. This means there's an assumption that the returned prop names don't change conditionally (but that assumption is I think something almost "granted" for withDispatch). This proved to be a big win in terms of performance.

This could still be done with connect.

The second performance gain we operated is the "Async" mode. AFAIK, react-redux don't have a support for that kind of subscriptions and I'm not sure it will be easy to implement above react-redux without touching its core.

Won't this be taken care of by React Async Rendering?

It could also be implemented at the store level. I.e. have the store keep a queue of dispatched actions, instead of having each component keep a queue of store updates. Also, why is that a queue? Shouldn't the component just render the latest update when it can?

[youknowriad]

@epiqueras you might be interested in reading the discussions in that PR https://github.com/WordPress/gutenberg/pull/13056

Won't this be taken care of by React Async Rendering?

The answer is no, explanations here https://github.com/WordPress/gutenberg/pull/13056#issuecomment-452720705

[youknowriad]

This could still be done with connect.

Do you have an example, curious to know more.

It could also be implemented at the store level.

That's not what the async mode is about because we still want important components to render sync (the current block) while less-priority components can be delayed (unselected blocks)

[epiqueras]

The answer is no, explanations here #13056 (comment)

That makes sense, thanks.

Do you have an example, curious to know more.

Actually, for withDispatch, it wouldn't make sense. It doesn't need to re-run on store changes so the current approach is fine.

That's not what the async mode is about because we still want important components to render sync (the current block) while less-priority components can be delayed (unselected blocks)

This could be a parameter of the subscriber (withSelect) that gets saved in the registry. I still don't get why it's a queue. When the main thread does clear up, shouldn't it just render the latest state?

[youknowriad]

I still don't get why it's a queue. When the main thread does clear up, shouldn't it just render the latest state?

Also, there are some discussions about this in the thread. The idea is if we have 1000 withSelect calls to perform, we won't run them all at once, we'll take items from the queue until there's room for it in the browser (if a more priority task comes after we run 200 items, we should run it first before resuming the queue). This is done using requestIdleCallback

[epiqueras]

Yeah, but the store is immutable. Why run all the intermediate updates and not just the latest one?

[youknowriad]

Why run all the intermediate updates and not just the latest one?

Oh we run just the latest ones, this is a "special" queue that allows us to "replace" items in the queue if the come from the same component.

[gziolo]

A bit related issue raised by @iandunn in #15244 which might shed some lights on what developers expect:

Is your feature request related to a problem? Please describe.
Redux is painful.

It may be a great choice for building a complex app like Calypso or Gutenberg, but it seems like complicated, verbose overkill for 90% of block development.

Describe the solution you'd like

What do people think about some kind of alternative interface that simplifies things. Here's a rough idea of the kind of interface I'm imagining:

const { fetch, update } = wp.data;

class TodoApp extends Component {
  render() {
      return (
          <TodoItems
              items={ fetch( 'todoapp', 'items' ) }
              // ^ makes HTTP GET request to `wp-json/todoapp/items/`
          />
      );
  }
}

class TodoItems extends Component {
  handleDelete( item ) {
      update( 'todoapp', 'items', {
          action: 'delete',
          id: item.id
      } );
      // ^ Makes HTTP DELETE request to `wp-json/todoapp/items/{item.id}`,
      // then waits for response, then triggers re-render() of TodoItems with updated `items` prop
  }

  render() {
      const { items } = this.props;

      return (
          <Fragment>
              { items.map( item => {
                  return (
                      <Item onDelete={ item => { this.handleDelete( item ) } } />
                  );
              } ) }
          </Fragment>
      );
  }
}

I'm sure there's lots of details to think through, and maybe some reasons why something like that isn't possible, but it feels like it's worth exploring, since it would greatly improve one of the most painful parts of block development today.

And of course, devs who want/need the full Redux feature set could still continue using the current abstraction layer.

It seems that React hooks make it possible to get very close to this proposal.

[nerrad]

It seems that React hooks make it possible to get very close to this proposal.

They do. Outside of our work on implementing things like useSelect, useDispatch etc., there's nothing precluding developers from creating their own custom hooks for things like fetch etc where they don't want to use the wp.data api. In general, I would recommend developers in the react ecosystem to learn hooks deeply ;)

[aduth]

Is it safe to say this can be closed with the merging of #15737 and #15896 ?

[nerrad]

yup, forgot about this :)