Add useQuerySelect to core/data #38134
Conversation
|
Size Change: +544 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
adamziel
added
the
Developer Experience
There was a problem hiding this comment.
One thing I dislike a bit is that registry.resolveSelect( store ).selector() and useResolveSelect( ( resolve ) => resolve( store ).selector() ) return very different things. One returns a promise, the other a { data, ...progressMetadata } structure.
It's unlike useSelect which is perfectly symmetrical with registry.select.
Maybe a different name, like useQuerySelect, is better? In the sense I described above, useResolveSelect is a bit misleading name 🙂
Other than that, this new hook is nicely compatible with the potential useSuspenseSelect hook. They all differ only in what the bound selectors return:
useSelectselectors return synchronously the current data in storeuseResolveSelectselectors return the sync data plus progress metadata in one packageuseSuspenseSelectselectors return the resolved data and throw if the selector is unresolved
|
Some first thoughts:
So instead of this: const { data, isResolving, hasFinished } = useResolveSelect(
( resolve ) => resolve( coreStore ).getEntityRecord( 'postType', 'page', pageId ),
[ pageId ]
};We should probably just do this (as stated in the PR description): const { data, isResolving, hasFinished } = useEntityRecord( 'postType', 'page', pageId );Resolving data is almost only useful for entity records, I don't see the use cases for using it for stores other than What about const useWidgets = () => useEntityRecords( 'root', 'widget', { per_page: -1, _embed: 'about' } );You can then perform other memoization or transformations using This is just my opinion though, and I'm still very happy with what this PR proposes (thank you again!). WDYT? |
I agree,
I agree. I don't see that many matches for I like the API you suggested, with one exception: I'd rather have two separate functions than one reasoning about the argument data type. I wonder if it would make sense to implement that on top of |
I think that |
Great synchrony! I was working on just that. This PR now implements Since there comments here are favorable, I will start cleaning things up and add unit tests, documentation, and perhaps some type definitions. |
|
I'll skip the detailed type definitions for now and go with similar ones as useSelect. More rigid typing would have to start with useSelect and is out of scope for this PR. Here's what I played with in case i could come handy one day: type Select<Config> = (StoreDescriptor<Config>) => SelectorsOf<StoreDescriptor<Config>>
type MapSelect<T> = (Select, DataRegistry) => T
type useSelect<T> = (MapSelect<T>, any[]) => T
type QuerySelectorsOf< Config extends AnyConfig > = Config extends ReduxStoreConfig<
any,
any,
infer Selectors
>
? { [ name in keyof Selectors ]: Function }
: never;
type QuerySelect<Config> = (StoreDescriptor<Config>) => QuerySelectorsOf<StoreDescriptor<Config>>
type MapQuerySelect<T> = (QuerySelect, DataRegistry) => T
type useQuerySelect<T> = (MapQuerySelect<T>, any[]) => T |
adamziel
force-pushed
the
propose/use-resolve-select
branch
from
26ee0b2 to
72e64e2
Compare
|
This one is ready for a review! CC @jsnajdr @kevin940726 |
adamziel
force-pushed
the
propose/use-resolve-select
branch
from
447b5f2 to
5ae67d7
Compare
adamziel
force-pushed
the
propose/use-resolve-select
branch
from
4e162c0 to
4b51ec9
Compare
There was a problem hiding this comment.
TBH, I still don't think we need this hook. IMO, we only need a high level API (useEntityRecord) and a low level API (useSelect) but this hook is something in between. Sure, it can help us create hooks for stores other than core-data, but given that these hooks are still experimental, I'm not sure if it's really helpful to create multiple hierarchies now.
I'm not against it though it people really think this is helpful now and are willing to expect that things could change 😅 .
|
|
||
| interface QuerySelectResponse { | ||
| /** the requested selector return value */ | ||
| data: Object; |
There was a problem hiding this comment.
Do we want to change this to generic too?
| data: Object; | ||
|
|
||
| /** is the record still being resolved? Via the `getIsResolving` meta-selector */ | ||
| isResolving: boolean; |
There was a problem hiding this comment.
Since this is a lower-level API, I'd imagine it to include status as well.
| hasStarted: boolean; | ||
|
|
||
| /** has the resolution finished? Via the `hasFinishedResolution` meta-selector. */ | ||
| hasResolved: boolean; |
There was a problem hiding this comment.
As mentioned in #38522 (comment), I'm not sure what's the difference between this and !!data? We can discuss this in the original thread though.
| return useSelect( ( select, registry ) => { | ||
| const resolve = ( store ) => enrichSelectors( select( store ) ); | ||
| return mapQuerySelect( resolve, registry ); | ||
| }, deps ); |
There was a problem hiding this comment.
IMO, adding deps to a custom hook is a bad idea. I wonder if we could somehow discourage it in this API by always returning the latest mapQuerySelect possible? Something like:
const latestMapQuerySelectRef = useLatestRef( mapQuerySelect );
return useSelect( ( select, registry ) => {
const resolve = ( store ) => enrichSelectors( select( store ) );
return latestMapQuerySelectRef.current( resolve, registry );
}, [] );There was a problem hiding this comment.
Wait, that's just a proxy to useSelect, what's wrong with having deps?
| if ( META_SELECTORS.includes( selectorName ) ) { | ||
| continue; | ||
| } | ||
| Object.defineProperty( resolvers, selectorName, { |
There was a problem hiding this comment.
Any reason why we're using Object.defineProperty rather than simply mutating the object? I guess TS won't generate the correct typings for this?
There was a problem hiding this comment.
It's because we're defining lazy getters that won't be evaluated until they're called for the first time.
| export const META_SELECTORS = [ | ||
| 'getIsResolving', | ||
| 'hasStartedResolution', | ||
| 'hasFinishedResolution', | ||
| 'isResolving', | ||
| 'getCachedResolvers', | ||
| ]; | ||
|
|
||
| const META_ACTIONS = [ | ||
| 'startResolution', | ||
| 'finishResolution', | ||
| 'invalidateResolution', | ||
| 'invalidateResolutionForStore', | ||
| 'invalidateResolutionForStoreSelector', | ||
| ]; |
There was a problem hiding this comment.
Nit: We can move these to constants.ts and type them as const to get the desired typings.
export const META_SELECTORS = [
'getIsResolving',
'hasStartedResolution',
'hasFinishedResolution',
'isResolving',
'getCachedResolvers',
] as const;
|
Should we close this PR now that The alternative I saw discussed in other places would be to rebase this PR and move the hook to |
|
Yes, let's close this PR @gziolo. I believe |
|
The code is there so it should be mostly moving code from one place to another 💯 |
Description
This PR proposes a new hook called
useResolveSelect. It is used like this:The rationale is to make snippets like below more readable and faster to write – I find myself looking up the correct way to work with resolution status every time:
In #38135 I also drafted a higher-level hook called
useEntityRecordbuilding on top of this PR:@youknowriad's work on the suspense API support in #37261 is related, but I see it as a separate API that doesn't diminish the value of having
useResolveSelect.Testing steps
This is a new hook so there's not much to test in the UI. Here's an alternative test plan:
cc @kevin940726 @talldan @gziolo @draganescu @ellatrix @noisysocks @jsnajdr @getdave