[data] Fill out type definition for data registry #39081
Conversation
|
Size Change: 0 B Total Size: 1.21 MB ℹ️ View Unchanged
|
| * This type is a convenience wrapper for turning that reference into | ||
| * an actual store regardless of what was passed. | ||
| * | ||
| * Warning! Will fail if given a name not already registered. In such a |
|
This is great @dmsnell, thank you for working on it!
For posterity, here's a snippet I used for testing: const actions = {
dispatchMyAction: ( nb: number ) => {
return 'test';
},
};
const selectors = {
testFn: () => {},
};
export interface StoreConfig
extends ReduxStoreConfig< any, typeof actions, typeof selectors > {}
export interface Stores extends DataStores {
test: StoreConfig;
}
const dr = {} as DataRegistry;
const thisShouldBeAString = dr.dispatch( 'test' ).dispatchMyAction( 12 );I noticed this doesn't yet support thunks , which means TS thinks the following action returns a function and not a number: const actions = {
iAmAThunk: ( nb: number ) => ( { select } ) => {
return 'test';
},
};
const result = dr.dispatch( 'test' ).iAmAThunk( 12 );
// Result is of type ( { select: any } ) => stringIt would be great to make that call return the string directly. If it would be easy to type the thunk argument, that's even better. How to identify a thunk? It's just any action or resolver at all that returns a function: gutenberg/packages/data/src/redux-store/thunk-middleware.js Lines 1 to 9 in 3da717b The following snippet has worked for me: type UnwrapThunks< Actions extends Record< string, Function > > = {
[ k in keyof Actions ]: Actions[ k ] extends (
...args: infer Args
) => ( ...nestedArgs: any ) => infer RetVal
? ( ...args: Args ) => RetVal
: Actions[ k ];
};
type test = UnwrapThunks< {
test: ( a: number ) => ( thunkArgs: any ) => string;
} >;
// test is ( a: number ) => stringWe could even introduce a ThunksArg type that these actions could use. Then we could require the nested function to have zero or one arguments of that type. One challenge with having such type, though, is the following use-case:
cc @jsnajdr |
|
It would also be great to reuse selectors type signatures with resolvers, e.g.: // selector
function getAuthors( state : State, query : Record<string, string> ) : Author[] | null { /* ... */ }
// resolver
export const getAuthors : ResolverOf< typeof selectors.getAuthors > = ( query ) => async ( { dispatch } ) => {
// ...
return undefined; // Resolvers typically don't return anything
}
// query is Record<string, string> by inference
// getAuthors is of type resembling ( query : Record<string, string> ) => ThunkOrValue<Promise<undefined>> |
4786f9d
to
5a908db
Compare
| ): NonNullable< | ||
| { | ||
| [ Name in keyof Selectors ]: CurriedState< Selectors[ Name ] >; | ||
| } | ||
| >; |
There was a problem hiding this comment.
Something about the type inference here isn't exact:
The error doesn't show up when I use the getEntityRecord selector directly without going through select.
I've had a similar problem in #39025 and ended up with one of these super long type declarations that test for every number of arguments separately:
type IsValidArg<T> = T extends object ? keyof T extends never ? false : true : true;
type ReplaceReturnType<T, TNewReturn> = T extends (a: infer A, b: infer B, c: infer C, d: infer D, e: infer E, f: infer F, g: infer G, h: infer H, i: infer I, j: infer J) => infer R ? (
IsValidArg<J> extends true ? (a: A, b: B, c: C, d: D, e: E, f: F, g: G, h: H, i: I, j: J) => TNewReturn :
IsValidArg<I> extends true ? (a: A, b: B, c: C, d: D, e: E, f: F, g: G, h: H, i: I) => TNewReturn :
IsValidArg<H> extends true ? (a: A, b: B, c: C, d: D, e: E, f: F, g: G, h: H) => TNewReturn :
IsValidArg<G> extends true ? (a: A, b: B, c: C, d: D, e: E, f: F, g: G) => TNewReturn :
IsValidArg<F> extends true ? (a: A, b: B, c: C, d: D, e: E, f: F) => TNewReturn :
IsValidArg<E> extends true ? (a: A, b: B, c: C, d: D, e: E) => TNewReturn :
IsValidArg<D> extends true ? (a: A, b: B, c: C, d: D) => TNewReturn :
IsValidArg<C> extends true ? (a: A, b: B, c: C) => TNewReturn :
IsValidArg<B> extends true ? (a: A, b: B) => TNewReturn :
IsValidArg<A> extends true ? (a: A) => TNewReturn :
() => TNewReturn
) : neverThere was a problem hiding this comment.
I think the error might be coming from the type for rememo there as I'm not seeing it. For my own testing I used this to get around having a real type for rememo, which reminds me, we should file a PR in DefinitelyTyped for it
const createSelector = rememoCreateSelect as <F>(selector: F, deps: any) => F;Screen.Recording.2022-03-18.at.2.21.13.PM.mov
you can also see in that screencast my original intended method for adding core styles into the type system. typeof storeConfig, or in this case, since storeConfig is a function that produces it, ReturnType<typeof storeConfig> so we don't even need to know about ReduxStoreConfig (though it's obviously fine to manually add ReduxStoreConfig<…, …, …>)
There was a problem hiding this comment.
@dmsnell Could it be that the selector types signature PR now stores these signatures in a separate file and doesn't associate them with the selector functions? Because this changed between my original comment and your reply. I'm asking because I noticed all the arguments are of the type any:
And I hoped to see kind: Kind, name: Name, key: KeyOf< EntityRecordType< Kind, Name > > etc
There was a problem hiding this comment.
Could it be that the #39025 now stores these signatures in a separate file and doesn't associate them with the selector functions?
I'm not sure what you mean here. This code is looking at the selectors directly and should report exactly what TypeScript knows about those selectors.
If you create ReduxStoreConfig<State, Actions, Selectors> manually then it might behave differently because we're telling the type system to trust that what we're saying is more accurate than what it infers. I don't think I understand your question exactly though, at least I am pretty sure I'm missing the question you are asking.
all the arguments are of the type any:
Since the selectors are written in JS we aren't getting types. This should match what info TypeScript has. As we add types or convert those modules to TypeScript the argument type information should appear automatically in this wrapped type. I'll try and get an example screencast of that.
There was a problem hiding this comment.
I'll try and get an example screencast of that.
What I meant was that once you wire the TypeScript type for the getEntityRecord selector instead of using the vanilla JS version, you will get the same signature error as on the original screenshot I posted above.
The reason it does not break like that at the moment is that the TS signature and the JS selector got decoupled in my PR after I initially reported the problem in this thread.
There was a problem hiding this comment.
I'm still not getting it, which may be because I just don't comprehend what's going on.
How does "a TS signature and JS selector [get] decoupled"?
There was a problem hiding this comment.
@dmsnell I meant the following timeline happened:
- I created TypeScript signatures for core-data selectors #39025 where I initially relied on
typeDef. ImportinggetEntityRecordmeant TypeScript knew that the first argument waskind: Kind, the second wasname: Nameand so on. - You created this PR
- I tested how the two work together and realized that the
@wordpress/datatype definitions here do not preserve the correct signature forgetEntityRecord– that's my comment above - I removed the
typeDefcomments from TypeScript signatures for core-data selectors #39025 and moved the type signatures into a separate.tsfile. After this moment, Typescript thinks thatgetEntityRecordtakes arguments of typeanybecause the full signature is not tied to the implementation in any way. In other words, importinggetEntityRecordmakes TS think that the first argument iskind: any, the second isname: any, and so on. - You were unable to reproduce the problem from point 3 because the PR TypeScript signatures for core-data selectors #39025 was no longer the same as when I reported the problem.
I believe that a version of the core-data store where getEntityRecord has the following signature:
export type getEntityRecord = <
R extends EntityRecordOf< K, N >,
C extends Context = DefaultContextOf< R >,
K extends Kind = KindOf< R >,
N extends Name = NameOf< R >,
/**
* The requested fields. If specified, the REST API will remove from the response
* any fields not on that list.
*/
Fields extends string[] | undefined = undefined
>(
state: State,
kind: K,
name: N,
key: KeyOf< R >,
query?: EntityQuery< C, Fields >
) =>
| ( Fields extends undefined
? EntityRecordOf< K, N, C >
: Partial< EntityRecordOf< K, N, C > > )
| null
| undefined;would not work correctly with the data types currently proposed by this PR. How? In the way I initially described in the root comment of this discussion.
5a908db
to
01a4918
Compare
|
@adamziel that's a good catch. I wonder what's best here but I went ahead and added thunk-unwrapping only for those action creators which returns functions taking I almost went back and relaxed the constraint to any function at all, but instead I just made |
07c6580
to
32d9653
Compare
Now I've gone back on going back and added a more generalized type. Noting here for a TODO that we also have "registry selectors" which accept |
The `DataRegistry` type has been a hollow shell of its real shape. In this patch we're adding basic types for all the methods and properties it exposes to the app. There is value to glean from expanding these types but as a starter this update should prevent someone from getting a false type error when attempting basic use of the registry in typed code.
32d9653
to
beca7cb
Compare
|
All: thanks for the review. Once the tests pass I'm going to merge this and get it in so we can start building off of it. I believe that for the most part we will not be introducing false type errors with this PR, apart from the potential when dealing with Planned followup:
|
|
I'm not able to merge this; is anyone willing to share in the journey and approve this PR? Or if you have objections, to mention them so I can address them? 🙇♂️ |
There was a problem hiding this comment.
Things are mostly looking okay for me, but I was having issues getting types for resolvers to work as I expected.
The error I'm getting is that that the async function that resolvers return isn't expected in the type. For example, it's expecting (s: string) => Promise<void> instead of (s: string) => ({ dispatch }) => Promise<void> in the store config.
Example Code
const actions = {
testDispatchAction: ( n: number ) => {
return {
type: 'TEST' as const,
n,
};
},
testDispatchAction2: ( s: string ) => {
return {
type: 'TEST2' as const,
s,
};
},
};
const selectors = {
testSelector: ( state: number, s: string ) => {
return s + state;
},
testSelector2: ( state: number ) => {
return state;
},
};
const resolvers = {
testSelector: ( s: string ) => async ( {
dispatch,
}: {
dispatch: any; // Could be typed better
} ) => {
dispatch.testDispatchAction2( s );
},
testSelector2: () => async ( { dispatch }: { dispatch: any } ) => {
dispatch.testDispatchAction2( 'hello' );
},
};
type ValueOf< A > = A[ keyof A ];
const reducer = (
state: number,
action: ReturnType< ValueOf< typeof actions > >
) => {
switch ( action.type ) {
case 'TEST':
return state + action.n;
case 'TEST2':
return state + Number( action.s );
default:
return state;
}
};
function storeConfig(): ReduxStoreConfig<
number,
typeof actions,
typeof selectors
> {
return {
reducer,
actions,
selectors,
resolvers, // First error here
};
}
export interface Stores extends DataStores {
testStore: ReturnType< typeof storeConfig >;
}
const dr = {} as DataRegistry;
const resolveSelect = await dr.resolveSelect( 'testStore' );
const resolveSelectRes = await resolveSelect.testSelector( 'hello' ); // Second error here
const select = dr.select( 'testStore' );
const selectResult = select.testSelector( 'hello' );
const dispatch = dr.dispatch( 'testStore' );
const dispatchResult = dispatch.testDispatchAction( 42 );
const unsubscribe = dr.subscribe( console.log.bind( console ) );
unsubscribe();
console.log( resolveSelectRes, selectResult, dispatchResult );Error Message
$ tsc --build
packages/data/src/types.ts:330:3 - error TS2322: Type '{ testSelector: (s: string) => ({ dispatch, }: { dispatch: any; }) => Promise<void>; testSelector2: () => ({ dispatch }: { dispatch: any; }) => Promise<void>; }' is not assignable to type '{ testSelector?: ((s: string) => Promise<string>) | undefined; testSelector2?: (() => Promise<number>) | undefined; }'.
The types returned by 'testSelector(...)' are incompatible between these types.
Type '({ dispatch, }: { dispatch: any; }) => Promise<void>' is missing the following properties from type 'Promise<string>': then, catch, finally, [Symbol.toStringTag]
330 resolvers,
~~~~~~~~~
packages/data/src/types.ts:41:2
41 resolvers?: ResolversOf< Selectors >;
~~~~~~~~~
The expected type comes from property 'resolvers' which is declared here on type 'ReduxStoreConfig<number, { testDispatchAction: (n: number) => { type: "TEST"; n: number; }; testDispatchAction2: (s: string) => { type: "TEST2"; s: string; }; }, { testSelector: (state: number, s: string) => string; testSelector2: (state: number) => number; }>'
packages/data/src/types.ts:340:46 - error TS2339: Property 'testSelector' does not exist on type '{} | Required<{ testSelector?: ((s: string) => Promise<string>) | undefined; testSelector2?: (() => Promise<number>) | undefined; }>'.
Property 'testSelector' does not exist on type '{}'.
340 const resolveSelectRes = await resolveSelect.testSelector( 'hello' );
~~~~~~~~~~~~
Found 2 errors.
I understand that it will make for a larger PR, but I'd like to see these types being used in actual code somewhere. The work is going to need to be done anyway, and it's much easier to test types when they're used and you can click through them in the context of where thy will be used.
| export interface DataStores {} | ||
|
|
||
| export interface Stores extends DataStores {} | ||
|
|
There was a problem hiding this comment.
What is the purpose of extending Stores from DataStores?
| type ResolvedThunks< Actions extends AnyConfig[ 'actions' ] > = { | ||
| [ Action in keyof Actions ]: Actions[ Action ] extends ( | ||
| ...args: infer P | ||
| ) => ( ...thunkArgs: any ) => infer R |
There was a problem hiding this comment.
| ) => ( ...thunkArgs: any ) => infer R | |
| ) => ( ...thunkArgs: any[] ) => infer R |
Minor change, but it looks like you're using any[] elsewhere, so it could be used here too.
| export interface Unsubscriber { | ||
| (): void; | ||
| } |
There was a problem hiding this comment.
Is there any reason that you're using an interface here instead of the more readable type declaration?
| export interface Unsubscriber { | |
| (): void; | |
| } | |
| export type Unsubscriber = () => void; |
|
@dmsnell I've got these types to work with core-data on my local working copy, but I've also ran into an issue with currying: // Minimum setup:
interface CommentEntityRecord{ id: number; content: string; }
interface PostEntityRecord{ id: string; title: string; }
type EntityConfig =
| { kind: 'root', record: CommentEntityRecord, keyType: number }
| { kind: 'postType', record: PostEntityRecord, keyType: string }
// Minimal reproduction of getEntityRecords
const getEntityRecords = <K extends EntityConfig['kind']>(
kind: K,
key: Extract< EntityConfig, { kind: K } >['keyType']
) : Extract< EntityConfig, { kind: K } >['record'] => { return {} as any; };
const comment = getEntityRecords('root', 15);
// ^? CommentEntityRecord
// Great! It's what we wanted
const clonedGetEntityRecords = {} as (...args:Parameters<typeof getEntityRecords>) => ReturnType<typeof getEntityRecords>;
const clonedComment = clonedGetEntityRecords('root', 15);
// ^? CommentEntityRecord | PostEntityRecord
// Oops, we don't want a union type here
// Also:
type Params = Parameters<typeof getEntityRecords>
// ^ ? [kind: "root" | "postType", key: string | number]
// But we know the signature is more complex than thatIt looks like copying a function with interdependent generic arguments in TypeScript is more involved than just using The following SO tackles a similar issue for the |
This commit adds TypeScript type definitions to `useSelect` via jsDoc annotations. Effectively, it give us autocompletion support: https://user-images.githubusercontent.com/205419/172873582-0de54185-7bdd-4f33-9b37-052cba954f5a.mp4 It's a step towards #39081, only it focuses just on `useSelect` Using the annotations in the same file where they're defined works flawlessly. However, after importing useSelect in another file, autocompletion doesn't work correctly. I believe it's because DefinitelyTyped definition get in the way. To confirm that hypothesis, I set up a separate repository with clean slate and no Lerna packages. Suddenly, all the annotations worked flawlessly when imported. I would like to make it work across Gutenberg packages but let's explore that in follow-up PR.
* Add SelectorWithCustomCurrySignature, a utility type to provide a custom curry signature for selectors when TypeScript inference falls short. * Add the PR link to the docstrings * Update packages/data/src/types.ts
Description
The type for the data registry created with
createRegistry()has lived asplit and minimal life: split across a basic type declaration in
types.tsand the associated JSDoc
@typedefinregistry.js; and minimal becauseneither definition comprehensively describes the structure.
In this patch we're unifying the type into the TypeScript declaration and
also filling it out to comprehensively describe the registry. The complicated
type in this patch is the result of several different attempts to provide the
following goals:
select()anddispatch()calls.core registry: no new type annotations in packages outside of
@wordpress/dataand no duplicating type information about other packages inside of
@wordpress/data.stores and not be required to give up any of the core store information.
In this patch the data registry is empty because I want to focus on the type itself.
I believe that adding additional core stores will require at a minimum, renaming some files to end in
.tsand activating TypeScript for that package if it isn't already.Testing Instructions
As a type-only change there should be no impact on the built files.
For testing and review please audit the code and types.
Screenshots
While not enabled in this PR itself, by using
typeof storeConfigin other packages we can get the list of actions and selectors (with thestateparameter curried away) for that store. The arguments and return types won't be typed because they only exist as JS, but we still get their names and the names of their argument and as we add typing to them we'll benefit through the registry.Screen.Recording.2022-02-23.at.9.20.48.PM.mov
Screen.Recording.2022-02-24.at.12.29.51.PM.mov
Types of changes
Updating an existing type definition and merging a JSDoc
@typedefinto it.Checklist:
*.native.jsfiles for terms that need renaming or removal).