-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
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.
* | ||
* @return {Function} Unsubscribe function. | ||
*/ | ||
const subscribe = ( listener ) => { | ||
listeners.push( listener ); | ||
const subscribe = ( listener, storeDependencies ) => { |
There was a problem hiding this comment.
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?
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. |
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 |
@youknowriad made this comment in slack that expands on why
So ya, unless implementors know that those selectors might invoke selectors from other stores, they might include the wrong dependencies. |
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 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 |
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. |
It seems a requirement of these changes anyways (store name as argument to |
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
anduseSelect
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?
Types of changes
This is a non-breaking change in that existing behaviour of subscribes is preserved.
Checklist: