Data: Use store instance as param for select and dispatch #26655
Conversation
gziolo
added
[Type] Technical Prototype
|
Size Change: +449 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
I went too far with removing |
I like that. Never understood why we have the hardcoded keys instead of imports. |
gziolo
force-pushed
the
try/data-store-as-param
branch
from
cf67b08
to
e3562e7
Compare
gziolo
force-pushed
the
try/data-store-as-param
branch
from
e3562e7
to
45c3475
Compare
gziolo
requested review from
ajitbohra,
atimmer,
cameronvoell,
draganescu,
ellatrix,
etoledom,
guarani,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
youknowriad
force-pushed
the
try/data-store-as-param
branch
from
ede9269
to
fcd8c78
Compare
|
I'm a bit late to the party, but for me too the main thing on my mind was:
So, nothing in the API would change for third-party consumers of Finally, just a side comment on:
I still prefer the simple string-based approach, but for me this question of "store objects" is exactly the kind of problem that ESNext's const storeSymbol = Symbol( 'core/foo' );
assert( storeSymbol !== Symbol( 'core/foo' ) );
assert( storeSymbol.description === 'core/foo' );
function select( storeName ) {
if ( typeof storeName === 'symbol' ) {
storeName = storeName.description;
}
/* ... */
} |
youknowriad
added
[Type] New API
|
for me the symbol type is the perfect thing to use when you want something to be "internal" to check against without a way for third-party devs to hack around it. While that was the original idea of the |
|
|
||
| _Returns_ | ||
|
|
||
| - (unknown type): Store Object. |
There was a problem hiding this comment.
It's a bit unfortunate that it isn't able to properly pick up the TS type.
| } ); | ||
|
|
||
| expect( store.getState() ).toBe( 'OK' ); | ||
| await registry.dispatch( STORE_NAME ).update(); |
There was a problem hiding this comment.
This test needs to be updated, to work with the store definition.
There was a problem hiding this comment.
I left this is as because it was using the redux store directly to access state.
Another related task would be to do the work required by the The Then, some other packages, like The The And so on for all other affected packages... |
Thanks! 🎉
Even if the |
|
The good news is that I'll double-check if we can also get rid of Update: @youknowriad, what do you think? |
|
I also opened a follow up that is meant to track refactoring of existing usage of hardcoded store keys: #27088. |
youknowriad
removed
the
Needs Dev Note
Description
I started a quick PR that could help to mitigate the issue with import statements for the packages that expose stores used in the package. In the long run, it isn't solid enough, which I was able to confirm by only testing one package
@wordpress/viewport. I propose that we expose the store object from every package and use it as a param that gets passed toselectanddispatchcalls rather than hardcoded strings. To make it backward compatible we can injecttoStringmethod that returns the name of the registered store and cast the store object when passing toselectanddispatchcalls.This PR also resolves #14150 where it was proposed to use consistent, accurate naming for store keys, I picked
keyfor now, but I'm happy to update tostoreNameas originally proposed.@wordpress/viewport package
Based on the fact that
@wordpress/viewportis no longer referenced in the codebase, it looks like@wordpress/viewportas a package is no longer necessary and it probably should be replaced with something else. Let's ignore this fact when discussing this package and focus on how we can ensure that data stores are properly imported by packages that use them.How has this been tested?
New unit tests were added.
Types of changes
Checklist: