TypeScript signatures for core-data selectors #39025
Conversation
adamziel
added
[Package] Core data
adamziel
changed the title
|
Size Change: -299 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Whenever I'm working on complicated types like this I like to watch my own code and check for an abundance of special-casing and awkward conventions and if I see that, think about if I'm pushing too hard on approach vs. stepping back to check if there might be others. We have two really obvious approaches with In this PR we're shooting for the first one, but obviously there's a ton of machinery in there to make it work, and some remaining questions about the ease of extending it. There are questions about the relationship between The other approach is almost too simple to warrant discussion. export const getEntityRecord = <T extends EntityRecord<any>>(…): T | null => { … }
const post = getEntityRecord<Post<'edit'>>('postType', 'post', 13, { context: 'edit' } );Every question about extending this is already taken care of because it's up to you to supply the return type (as the caller). I hope that we can get to full auto-typing and inference, but what I'm wondering is if there's a way to step there that starts with the simpler approach, specifically of having the const post = getEntityRecord<'postType', 'post', 13, 'edit'>('postType', 'post', 13, { context: 'edit' } );Is this possible to try and look for a type signature for full inference that starts with export const getEntityRecord = <C extends Context, T extends EntityRecord<C>>(
state: State,
kind:
)For example, I believe that this gets us the expected type KindOf<T extends EntityRecord<any>> = keyof OmitNevers<{
[Kind in keyof EntityRecordLookup['root']]:
EntityRecordLookup['root'][Kind] extends T
? T
: never;
}>;
The great thing about extensibility is that if we can get to the point where this is parameterized by one required type parameter, we should be able to provide automatic inference for all the core types without imposing any difficult steps on people to extend it. Maybe they don't want to go overwrite ambient namespaces types; they can supply their own type parameter and call it a day. We can potentially do this through conditional types checking if the given EntityRecord is known or not. export const getEntityRecord = <T extends EntityRecord<any> | CustomEntityRecord>(…): T | null { … }
Screen.Recording.2022-02-23.at.1.00.43.PM.movLots of doodling, but I'm just trying to see how far we can get with how little in the terms of type parameters and annotation. |
|
My brain is fried so just a quick note – I think the full inference works (although it's based on kind and type)!
And the types aren't even that complicated: export type EntityType< C extends Context = any > =
| DeclaredEntity< 'root', 'site', Settings< C > >
| DeclaredEntity< 'root', 'postType', Type< C > >
| DeclaredEntity< 'root', 'media', Attachment< C > >
| DeclaredEntity< 'root', 'taxonomy', Taxonomy< C > >
| DeclaredEntity< 'root', 'sidebar', Sidebar< C > >
| DeclaredEntity< 'root', 'widget', Widget< C > >
| DeclaredEntity< 'root', 'widgetType', WidgetType< C > >
| DeclaredEntity< 'root', 'user', User< C > >
| DeclaredEntity< 'root', 'comment', Comment< C > >
| DeclaredEntity< 'root', 'menu', NavMenu< C > >
| DeclaredEntity< 'root', 'menuItem', NavMenuItem< C > >
| DeclaredEntity< 'root', 'menuLocation', MenuLocation< C > >
| DeclaredEntity< 'root', 'navigationArea', NavigationArea< C > >
| DeclaredEntity< 'root', 'theme', Theme< C > >
| DeclaredEntity< 'root', 'plugin', Plugin< C > >
| APIEntity< 'postType', 'post', Post< C > >
| APIEntity< 'postType', 'page', Page< C > >
| APIEntity< 'postType', 'wp_template', WpTemplate< C > >
| APIEntity< 'postType', 'wp_template_part', WpTemplatePart< C > >;
export type Kind = EntityType[ 'kind' ];
export type Name = EntityType[ 'name' ];
export type EntityRecordType<
K extends Kind,
N extends Name,
C extends Context = any
> = Extract< EntityType< C >, { kind: K; name: N } >[ 'recordType' ];
export type Key< K extends Kind, N extends Name > = Extract<
EntityType,
{ kind: K; name: N }
>[ 'keyType' ];
export type DefaultContext< K extends Kind, N extends Name > = Extract<
EntityType,
{ kind: K; name: N }
>[ 'defaultContext' ];My brain is fried so that's all I'm going to write right now, but my next comment will address the specific points you brought up @dmsnell -- edit: Here's the full EntityType-based inference: type Context = 'edit' | 'update' | 'view' | 'embed';
interface Post < C extends Context > { brand: 'post' }
interface Comment < C extends Context > { author: 2 }
interface Template < C extends Context > { distinct: 84 }
type EntityRecord < C extends Context > = Post<C> | Comment<C> | Template<C>
type State = any;
type EntityType< C extends Context > =
| { kind : 'root', name: 'post', recordType: Post<C> }
| { kind : 'root', name: 'comment', recordType: Comment<C> }
| { kind : 'postType', name: 'template', recordType: Template<C> }
export type RecordOf< Kind, Name, C extends Context > = Extract<
EntityType<C>,
{ kind: Kind, name: Name }
>['recordType']
export type KindOf< R extends EntityRecord<any> > = Extract<
EntityType<any>,
{ recordType: R }
>[ 'kind' ];
export type NameOf< R extends EntityRecord<any> > = Extract<
EntityType<any>,
{ recordType: R }
>[ 'name' ];
export function getEntityRecord<
R extends RecordOf< Kind, Name, C >,
C extends Context='edit',
Kind extends EntityType<C>['kind']=KindOf<R>,
Name extends EntityType<C>['name']=NameOf<R>
>(
state: State,
kind: Kind,
name: Name,
query?: {
context: C
}
) : R { return {} as any; }
const post1 = getEntityRecord({}, 'root', 'post')
const post2 = getEntityRecord({}, 'root', 'post', { context: 'view' })
const post3 = getEntityRecord<Post<'edit'>>({}, 'root', 'post')
const post4 = getEntityRecord<Post<'view'>, 'view'>({}, 'root', 'post', { context: 'view' }) |
|
I reduced this PR to focus just on the
Great point! This would "just work" and solve a ton of problems here. The full inference is just so tempting, though 😆 I reduced this PR to something much simpler than it used to be, combining the first version with your explorations, which enabled the following signature: export const getEntityRecord = function <
R extends EntityRecordType< K, N, C >,
K extends Kind = KindOf< R >,
N extends Name = NameOf< R >,
C extends Context = DefaultContextOf< K, N >
>(
state: State,
kind: K,
name: N,
key: PrimaryKey< R >,
query?: EntityQuery< C >
): R | null | undefined Unfortunately, I couldn't the inference to work right without the K and N generic parameters. This how it is used: const commentDefault = getEntityRecord( {}, 'root', 'comment', 15 );
// commentDefault is Comment<'edit'>
const commentView = getEntityRecord( {}, 'root', 'comment', 15, {
context: 'view',
} );
// commentView is Comment<'view'>
const commentInvalidPK = getEntityRecord( {}, 'root', 'comment', '15' );
// commentInvalidPK shows a TypeScript error
const commentCustom = getEntityRecord< Comment< 'edit' > >( {}, 'root', 'comment', 15 );
// commentCustom is Comment<'edit'>
const commentCustomContext = getEntityRecord<
Comment< 'view' >,
'root',
'comment',
'view'
>( {}, 'root', 'comment', 15, {
context: 'view',
} );
// commentCustom is Comment<'view'>It requires the K and N parameters when you want to pass a custom Record type in a non-edit context. I'm not sure the // No bueno in either case
export const getEntityRecord = function <
R extends EntityRecord< C >,
C extends Context = 'edit'
>(
state: State,
kind: KindOf< R >,
name: NameOf< R >,
key: PrimaryKey< R >,
query?: EntityQuery< C >
): R
export const getEntityRecord = function <
R extends EntityRecord< C >,
C extends Context = 'edit'
>(
state: State,
kind: KindOf< R >,
name: NameOf< R >,
key: PrimaryKey< R >,
query?: EntityQuery< C >
): EntityRecordType< KindOf< R >, NameOf< R >, C > |
Keeping in mind that most WordPress contributors will probably be consuming this from regular JS and not have the ability to pass type parameters to functions, full inference would be a good goal to reach; we don't even have full TS support in the create-block generator yet. |
|
I boiled the generics signature down to: <
R extends RecordOf< Kind, Name, C >,
C extends Context=RContextOf<R>,
Kind extends EntityType<C>['kind']=KindOf<R>,
Name extends EntityType<C>['name']=NameOf<R>,
>Which is close to perfection – the record comes first, then the optional context, and then optional Kind and Type which can almost always be omitted. Here's the usage: const post1 = getEntityRecord({}, 'root', 'post')
// post1 is Post<'edit'>
const post2 = getEntityRecord({}, 'root', 'post', { context: 'view' })
// post2 is Post<'view'>
const post3 = getEntityRecord<Post<'edit'>>({}, 'root', 'post')
// post3 is Post<'edit'>
const post4 = getEntityRecord<Post<'view'>, 'view'>({}, 'root', 'post', { context: 'view' })
// post4 is Post<'view'>One downside I see is that the R parameter is not a type EntityRecord < C extends Context > = Post<C> | Comment<C> | Template<C>
type EntityType< C extends Context > =
| { kind : 'root', name: 'post', recordType: Post<C>, defaultContext: 'edit' }
| { kind : 'root', name: 'comment', recordType: Comment<C>, defaultContext: 'view' }
| { kind : 'postType', name: 'template', recordType: Template<C>, defaultContext: 'view' }
// RecordOf depends on EntityTypeAnd this not extensible yet. We can fix that, though, by storing the entity types in an interface as below: // core-data code
type CoreEntityType< C extends Context > =
| { kind : 'root', name: 'post', recordType: Post<C>, defaultContext: 'edit' }
| { kind : 'root', name: 'comment', recordType: Comment<C>, defaultContext: 'view' }
| { kind : 'postType', name: 'template', recordType: Template<C>, defaultContext: 'view' }
export interface EntityTypeWrapper< C extends Context > {
core: CoreEntityType< C >
}
// Same as below, but now we read all the values stored in EntityTypeWrapper
type EntityType< C extends Context > = EntityTypeWrapper<C> [ keyof EntityTypeWrapper<C> ]
// This is no longer a hardcoded union type, but depends on the configuration
type EntityRecord < C extends Context > = EntityType<C>['recordType']
// Plugin code
interface Order { customerId: number; }
type myPluginEntityType< C extends Context > =
| { kind : 'myPlugin', name: 'order', recordType: Order, defaultContext: 'view' }
export interface EntityTypeWrapper< C extends Context > {
myPlugin: myPluginEntityType< C >
}
const order = getEntityRecord({}, 'myPlugin', 'order')
// order is OrderIn this case, One thing this setup doesn't allow is completely arbitrary return types as in |
|
because I have a bunch of changes in my local working copy I'm not going to jump into this now but will come back to it in a bit. I like where it's going; I want to see if I can figure out a way to get rid of the regardless this is going to be great and I'm glad we're pushing hard on some complicated internal types because of the way it will let plugin authors and core contributors outside of this package get all the type benefits with little to no annotation or work on their part. |
This is super clever, I would not have considered this approach at all. Definitely putting that one in my back pocket. |
|
This is where we stand right now: export const getEntityRecord = createSelector(
function <
R extends RecordOf< K, N >,
C extends Context = DefaultContextOf< R >,
K extends Kind = KindOf< R >,
N extends Name = NameOf< R >
>(
state: State,
kind: K,
name: N,
key: KeyOf< R >,
query?: EntityQuery< C >
): RecordOf< K, N, C > | null | undefined {
// ...
);
const commentDefault = getEntityRecord( {}, 'root', 'comment', 15 );
// commentDefault is Comment<'edit'>
const commentView = getEntityRecord( {}, 'root', 'comment', 15, {
context: 'view',
} );
// commentView is Comment<'view'>
const commentInvalidPK = getEntityRecord( {}, 'root', 'comment', '15' );
// commentInvalidPK shows a TypeScript error
const commentCustom = getEntityRecord<Comment<'view'>, 'view'>({},'root','comment',15,{ context: 'view' });
// commentCustom is Comment<'view'>Any big-picture improvements you see? If we're in the right ballpark, I'll start cleaning this PR up. |
|
Still having a pretty hard time following the types around here and figuring out the purpose of each of them and how exactly to think through making changes or maintaining them. I wonder if there's not a different approach that doesn't require so many layers of type wrapping. For instance, when trying to understand
Maybe the type that If we're making an Going to continue considering this. I'm a bit nervous about it the way it is at the moment, knowing we'll have to support it. As always, thanks for your persistence and hard work on this! |
adamziel
force-pushed
the
ts/type-getentityrecords
branch
from
3c9d87f to
b9eb573
Compare
@dmsnell You've definitely got a point here. I updated this PR to minimize the indirection as much as I could. I also made a distinction between entity records and entity types and added some nudges in the docstrings. I am not sure if this can be simplified much further besides shuffling things around.
Using
To make things more complex, {
name: 'menuLocation',
kind: 'root',
baseURL: '/wp/v2/menu-locations',
baseURLParams: { context: 'edit' },
plural: 'menuLocations',
label: __( 'Menu Location' ),
key: 'name',
}In terms of databases, we are compiling the following data: SELECT
kind,
name,
recordType,
DEFAULT(key, 'id') as key,
DEFAULT(baseURLParams['context'], 'view') as defaultContext
FROM defaultEntities_declared_in_entities_js d
INNER JOIN record_types r ON d.kind = r.kind AND d.name = r.name
UNION
SELECT VALUES
ROW ( 'postType', 'post', 'id', 'Post', 'edit' ),
ROW ( 'postType', 'page', 'id', 'Page', 'edit' ),
ROW ( 'postType', 'wp_template', 'WpTemplate', 'id', 'edit' ),
ROW ( 'postType', 'wp_template_part', 'WpTemplatePart', 'id', 'edit' ),
AS what_we_intrinsically_know_about_these_unconfigured_entites
UNION
SELECT
kind,
name,
recordType,
key,
defaultContext
FROM additional_entities_provided_by_extendersThis is quite a complex query and the underlying TS implementation reflects that complexity.
Do you mean something like below? type CoreEntity = {
root: {
comment: Comment<C>,
// ...
}
}That was my first instinct, too. I tried some variations of that, but it generated some additional hoops to jump through and the code ended up being more complex, not less. I've also tried a few things like redeclaring the
|
adamziel
force-pushed
the
ts/type-getentityrecords
branch
from
4752f0a to
27db5ca
Compare
|
@dmsnell I rebased this on top of latest trunk, standardized the language a bit, and removed a few unused types. |
In this commit we're cleaning up type issues in the core-data package that prevent us from telling TypeScript to run on the package and all of its existing code, even the JS files. After these changes we should be able to do so and start converting more modules to TypeScript with less friction. This patch follows a series of other smaller updates: - #39212 - #39214 - #39225 - #39476 - #39477 - #39479 - #39480 - #39525 - #39526 - #39655 - #39656 - #39659 It was built in order to support ongoing work to add types to the `getEntityRecord` family of functions in #39025.
In this commit we're cleaning up type issues in the core-data package that prevent us from telling TypeScript to run on the package and all of its existing code, even the JS files. After these changes we should be able to do so and start converting more modules to TypeScript with less friction. This patch follows a series of other smaller updates: - #39212 - #39214 - #39225 - #39476 - #39477 - #39479 - #39480 - #39525 - #39526 - #39655 - #39656 - #39659 It was built in order to support ongoing work to add types to the `getEntityRecord` family of functions in #39025.
…41235) A subset of #39025. Adds type signatures for the getEntityRecord and getEntityRecords selectors that supports the following use-cases: ```ts const commentDefault = getEntityRecord( {} as State, 'root', 'comment', 15 ); // commentDefault is Comment<'edit'> const commentView = getEntityRecord( {} as State, 'root', 'comment', 15, { context: 'view', } ); // commentView is Comment<'view'> const commentInvalidPK = getEntityRecord( {} as State, 'root', 'comment', '15' ); // commentInvalidPK shows a TypeScript error const commentView2 = getEntityRecord( {} as State, 'root', 'comment', 15, { context: 'view', _fields: [ 'id' ], } ); // commentView is Partial< Comment<'view'> > ```
|
This huge PR was entirely merged in small pieces listed in the description above. To make use of these type signatures useful outside of |
Description
This PR proposes a set of type signatures for selectors in core-data.
Because of the sheer volume of changes, I'm splitting this large PR into a series of smaller, atomic changes:
Overall, the challenges here can be illustrated using
getEntityRecordas an example:gutenberg/packages/core-data/src/selectors.js
Lines 127 to 136 in 0b1df88
And the type signature proposed in this PR:
Entity record types are associated with string-based kind and name. Type inference is enabled through
CoreEntity, a database-disguised-as-a-lookup-type that enables looking up (kind,name)->recordType and vice versa.Typing the entire core-data is way beyond this PR. This proposal paves the way by adding the appropriate type declarations to
src/types/selectors.tswithout bringing them over tosrc/selectors.jsyet.Test plan
Create a scratch file inside core-data and confirm the following data types check out:
The
Rgeneric is the entity record type, and is the primary mean of configuring the calls togetEntityRecord. TheC,K, andNgenerics would ideally be removed, but I don't see an easy way to do that.cc @dmsnell @jsnajdr @sarayourfriend @sirreal