[@wordpress/data] Add types to the useSelect function #37239
Conversation
|
Size Change: +8 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
| @@ -106,7 +109,7 @@ export default function useSelect( mapSelect, deps ) { | |||
| // `_mapSelect` if we can. | |||
| const callbackMapper = useCallback( | |||
| hasMappingFunction ? mapSelect : noop, | |||
| deps | |||
| deps ?? [] | |||
There was a problem hiding this comment.
This change is (weirdly) causing unit tests to fail 😅
|
Could something (pretty verbose and somewhat redundant) like this maybe work? (Just tinkering, I might even be totally misreading export type StoreConfigOf< T extends SelectChooser > = T extends (
select: ( reference: StoreDescriptor< infer Config > ) => MapOf< Function >,
registry: DataRegistry
) => unknown
? Config
: any;(ideally replacing and then export default function useSelect(
mapSelect: SelectChooser,
deps?: unknown[]
): SelectChooser extends SelectMapper
? any
: SelectorsOf< StoreConfigOf< typeof mapSelect > > { |
Oh, |
|
Ah, I think I got the order of the conditional type in the return type of export type StoreConfigOf<
T extends SelectChooser
> = T extends StoreDescriptor< infer Config > ? Config : any; |
|
@ockham thanks for the review!!!
Eventually that's where I'd like to go but I've had trouble getting there. In fact I have some uncommitted changes on my laptop I've been playing with but I frequently get back to the same point where the types seem to unify but fail to provide any insight. That is, the inferred types end up I'm not sure if I've done things wrong or TypeScript is giving up in these cases. You can inspect that in a new branch I just now pushed Thankfully I pushed this PR out before going further down that route because I felt like it was a dead end for now. In its place I'm hoping this helps some without trapping us later.
Another eventuality I hope, but for now that type is intentionally a bit ambiguous. We'll need to/want to be able to remove the |
Yeah, that's pretty much also the furthest I got after a bit of tinkering.
Can't speak for you, but with regards to the stuff I've tried, I'm thinking PEBCAK rather than TS is to blame 😬 My hypothesis is that some of the
Ahr yeah, some of that looks similar to my notes. Thanks for sharing!
Makes sense 👍 Well I sure don't object to merging this pretty-much as is. I think we need to update some docs that the linter is complaining about, but other than that --
🍛 👌 |
? 😄
Try using const foo = <Mapper extends SelectMapper<infer Config>>(mapper: Mapper): Config['selectors'] => {}We have to use those in the return type and we have to provide a type for the condition, even if it can't fail. type Goofy<A extends string> = A extends string ? A : never;And so if you think it's returning |
|
Thanks -- good point about the I've started to try and debug this by looking at some intermediate types. E.g. for my aforementioned import { store as coreStore } from '@wordpress/core-data';
/*...*/
type StoreConf = StoreConfigOf< typeof coreStore >;and then hover over |
431562f to
7f9e829
Compare
|
Having trouble debugging the I've tried running the script with |
|
API doc generation seems to fail when we are re-exporting some of the context providers without JSDoc types Works Doesn't work |
7f9e829 to
b3758b6
Compare
| registry: DataRegistry | ||
| ): unknown; | ||
| } | ||
| export type SelectChooser = SelectMapper | StoreReference; |
There was a problem hiding this comment.
This looks like something very specific to the useSelect hook and not a "meaningful" type. It's just a nitpick and I don't really have experience with typescript much but I'd have avoided a named typed for this one and just inlined it personally.
There was a problem hiding this comment.
this appears again in the type for DataRegistry for the select function. I was going to move it but forgot about that one. in time I'd like to come up with some better descriptions and names for these but I think for now at least we need to keep it here or duplicate the definition (which I'd rather avoid) so we don't cause false negatives on registry objects.
aee14c0 to
7f839c4
Compare
ef8e8bc to
cdcb1bd
Compare
|
Anyone have thoughts on this? It's getting pretty dusty now and I'd like to push forward with it and with work that depends on it, but I don't want to start that work before this is actually in. |
cdcb1bd to
1db052f
Compare
Description
Adds primitive TypeScript types to
useSelect. In this PR we're addingenough type information to
useSelectto make it (a) internally consistent,and (b) marginally useful to callers.
Internal consistency is mostly about introducing types without violations
so that TypeScript can compile the code successfully. While we have introduced
a few type casts in this patch they are either benign or replicate existing risk,
such as in asserting that an
errorin acatchclause does in fact extend theErrorclass/prototype.Marginally useful is about providing information to callers that ensures type
safety and introduces information to aid in development. In order to keep this PR
small the goals are low here. We don't provide any information about the return
value from calling
useSelectother than to indicate that it could be a map offunctions or it could be anything (if provided a mapping function). It does, however,
provide a hint for the mapping function and what arguments it expects.
There is much more we should be able to do to let our types flow through the
system and to provide hints on what functions are available after calling
select(store)but for now this makes one small step towards that goal while hopefully not
taking any steps backward away from it.
How has this been tested?
Screenshots
Types of changes
Adds types to
useSelectwith minimal changes to actual code.There should be no functional or behavioral changes in this PR,
apart from a couple of small fixes as forced by TypeScript, for
example, in supplying a default value for
depstouseCallbackwhen not supplied by the caller.
Checklist:
*.native.jsfiles for terms that need renaming or removal).