Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@wordpress/data: Allow for subscribing listeners to specific stores. #15483

Closed
wants to merge 1 commit into from

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented May 7, 2019

Description

This allows consuming code to pass in a list of stores the listener is specifically subscribing to. The listener will only get invoked when for any state changes on the dependent stores.

Default behaviour is preserved if no dependency array is passed in.

This behaviour will be useful in implementing store dependencies for withSelect and useSelect as a part of the explorations for #15473. There's also likely some performance improvements that can be gained via the store dependency argument.

How has this been tested?

  • ensure existing tests pass
  • added new tests for dependencies.

Types of changes

This is a non-breaking change in that existing behaviour of subscribes is preserved.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

This allows consuming code to pass in a list of stores the listener is specifically subscribing to.  The listener will only get invoked when for any state changes on the dependent stores.

Default behaviour is preserved if no dependency array is passed in.
@nerrad nerrad added [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts [Package] Data /packages/data labels May 7, 2019
@nerrad nerrad self-assigned this May 7, 2019
*
* @return {Function} Unsubscribe function.
*/
const subscribe = ( listener ) => {
listeners.push( listener );
const subscribe = ( listener, storeDependencies ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming question. I wonder if this should be dependentStores rather than what is here - storeDependencies might suggest it is dependencies for a store as opposed to a list of stores the subscription is dependent on?

@aduth
Copy link
Member

aduth commented May 7, 2019

Previously: 32068e8 (#13177)

The biggest challenge here is more in how it's used in practice. You could express that you depend on a particular store, but unless you know that the selectors you're using operate exclusively within the state of that store, those subscribers will not be called if the selector is a "registry selector" (#13662). As such, we may be providing people with a tool to shoot themselves in the foot.

@nerrad
Copy link
Contributor Author

nerrad commented May 7, 2019

You could express that you depend on a particular store, but unless you know that the selectors you're using operate exclusively within the state of that store, those subscribers will not be called if the selector is a "registry selector" (#13662).

But isn't that a problem right now anyways? If you subscribe to a given registry the listeners will only be invoked for that registry. Practically speaking, this implementation will mostly be used for withSelect and useSelect which will use the registry provided via the RegistryContext so the registry selectors in this case will be specific to it right?

@nerrad
Copy link
Contributor Author

nerrad commented May 7, 2019

@youknowriad made this comment in slack that expands on why createRegistrySelector creates problems for something like this:

Another problem here is the fact that we have "registry selectors". It means a selector can potentially depend on multiple stores even if defined in one

So ya, unless implementors know that those selectors might invoke selectors from other stores, they might include the wrong dependencies.

@nerrad
Copy link
Contributor Author

nerrad commented May 7, 2019

I wonder if instead of implementing dependency management, we could simply expose the store that had a state change when subscribe is invoked. Then client code that wants to implement their own store/state gates can utilize the provided store key in whatever additional logic it wants to do.

Another possibility is for things like createRegistrySelector the api for the function requires a dependency array (which I think is what @aduth was alluding to here). So something like this:

export function createRegistrySelector( registrySelector, dependentStores ) {
        if ( dependencies === undefined ) {
             // throw message about dependencies required for selector
        }
        registrySelector.dependantStores = dependentStores;
	registrySelector.isRegistrySelector = true;
	return registrySelector;
}

Then that dependency map would be connected with the store that selector belongs to so any dependency passed in for that store triggers including it's dependent stores. I imagine some linting might be possible in this case as well to ensure any usage of createRegistrySelector includes the right store dependencies as that arg.

@nerrad
Copy link
Contributor Author

nerrad commented May 7, 2019

After thinking about it, this is essentially a duplicate of #13177 so I'm going to close this in favor of the existing prior art happening on 13177.

@nerrad nerrad closed this May 7, 2019
@nerrad nerrad deleted the FET/subscribe-to-specific-store branch May 7, 2019 19:45
@aduth
Copy link
Member

aduth commented May 8, 2019

I wonder if instead of implementing dependency management, we could simply expose the store that had a state change when subscribe is invoked.

It seems a requirement of these changes anyways (store name as argument to subscribe callback), so certainly something likely to be needed. Not sure if it's something which has huge value on its own, or at least we ought to be wary to commit to an interface change until we're more certain of the bigger picture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants