unstableSubscribeStore: support store descriptors #45481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
|
Thanks for the ping. Yeah I agree, I would love to see the registry |
kevin940726
approved these changes
Nov 2, 2022
There was a problem hiding this comment.
LGTM 👍
What if we, instead of adding a new method, just overloaded the existing subscribe?
Or we could add a second argument to subscribe to specify which store(s) we want to subscribe to.
subscribe( listener, [ shopStore, profileStore ] );Pros:
- The first argument is the same, the API is easier to type and less confusing.
Cons:
- Kind of not what we're used to for a "subscribe"-like API.
I'm okay with both approaches though, just want to throw an idea out 😜 .
Specifying the store as a second argument is probably better, because we don't need to do tricks in the implementation like: function subscribe( store, listener ) {
if ( typeof store === 'function' ) {
listener = store;
store = undefined;
}
/* ... */
}This is also hard to type. I don't like the list of stores, though, because, first, I'd like to have consistency with other registry APIs that also take the store as an argument: registry.select( storeName );
registry.dispatch( storeName );
registry.subscribe( listener, storeName );Second, |
|
Sounds reasonable to me! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we want to stabilize the
__unstableSubscribeStoremethod, one of the requirements is to support store descriptors (objects) instead of just string store names. This PR adds that support.What if we, instead of adding a new method, just overloaded the existing
subscribe?subscribe( listener: () => void ): () => voidsubscribes to all storessubscribe( storeName: StoreNameOrDescriptor, listener )would subscribe just to one storeThis way the
subscribemethod is symmetrical to the existingregistry.select( storeName )andregistry.dispatch( storeName ). Every store, no matter how it's implemented, must support selecting, dispatching, and subscribing, and the registry methods are the standard way to access the individual stores.CC @alexflorisca