Entity-aware type signature for getEntityRecord and getEntityRecords #41235

adamziel · 2022-05-23T11:52:50Z

What problem does this PR solve?

A subset of #39025

This PR proposes a type signature for the getEntityRecord and getEntityRecords selectors that supports the following use-cases:

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'> >

And analogous use-cases for getEntityRecords.

Testing Instructions

Confirm the tests are green, except for what I mentioned above
Confirm the types work as described above
Confirm the types read well and make sense

cc @dmsnell @sarayourfriend @sirreal

adamziel · 2022-05-23T11:55:20Z

packages/core-data/src/selectors.ts

+
+// createSelector isn't properly typed if I don't explicitly import these files – ideally they would
+// be merely ambient definitions that TS is aware of.
+import type {} from './rememo';


I'm not sure what's the best way of using this ambient type without TS warnings before enabling "types" in package.json – these two seem to be connected.

Might be worth finally submitting a patch to rememo which adds a types.d.ts for it, or submitting a new package to the DefinitelyTyped repo. We keep getting back to the point of adding workarounds for it and I think now is as good of a time as ever to make the types real - they aren't that hard to write.

Adding types.d.ts would work but I worry it would also create the need to revisit/remove all the jsDoc annotations. I went for a lazy approach of merely adding a @return annotation. It did the trick in my local testing:

aduth/rememo#7

In the meantime, I introduced typed-rememo.ts to this PR so that we have a path forward in case merging that PR takes a long time.

Looks like y'all are on an older version of rememo (v3.0.0). First-party type definitions are available as of v4.0.0, so should hopefully be as simple as upgrading.

Since it's used across many different packages, I opened a separate pull request to upgrade them all in one go: #41415

I went ahead and merged that PR, thank you so much @aduth! I am AFK this week so I will rebase this one in a few days to confirm it went well.

packages/core-data/src/selectors.ts

dmsnell · 2022-05-24T22:25:29Z

packages/core-data/src/selectors.ts

+			 *
+			 * For more details, visit https://github.com/microsoft/TypeScript/issues/23132
+			 */
+			return filteredItem as any;


/me looks suspiciously at this comment

going to have to think about this a little. could we get around this by supplying two function type declarations? one in which fields is a string[] and which returns Partial<…> and another in which fields is missing and which returns EntityRecord<…>?

!! I like this idea! I just got it to work in TS playground:

type NumberToNumber = (arg:number) => number; type StringToString = (arg:string) => string; const eitherFunction = {} as NumberToNumber & StringToString; const numberHopefully = eitherFunction(1); // ^? number const stringHopefully = eitherFunction('abc'); // ^? string

I'll try to apply this to the selector signature as well.

@dmsnell I ended up getting nowhere with the type intersection operator. The signature overloading via the interface declaration as follows:

interface GetEntityRecord { < R extends EntityRecordOf< 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< K, N >, query: EntityQuery< C > ): Partial< EntityRecordOf< K, N, C > > | null | undefined; < R extends EntityRecordOf< 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< K, N >, query?: Omit< EntityQuery< C >, '_fields' > ): EntityRecordOf< K, N, C > | null | undefined; } const getEntityRecordImplementation: GetEntityRecord = < R extends EntityRecordOf< 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 > ) => {};

I don't love the proliferation of types here, but it does a good job of distinguishing between the two return types. Trade-offs, trade-offs everywhere 🤷 I've also inlined the getEntityRecordImplementation 1bd4111

✅ One declaration less
❌ as GetEntityRecord doesn't give us the same type safety as : GetEntityRecord

adamziel · 2022-05-26T15:34:22Z

@dmsnell I went for the getting the dependency graph of getEntityRecord to work and then also introduced the getEntityRecords type signature. I hoped to see all green checks, but there are some docgen issues yet again:

Error: Command failed with exit code 1: "/Users/cloudnik/www/Automattic/core/plugins/gutenberg/node_modules/.bin/docgen" packages/core-data/src/selectors.ts --output docs/reference-guides/data/data-core.md --to-token --use-token "Autogenerated selectors|../../../packages/core-data/src/selectors.ts" --ignore "/unstable|experimental/i"

TypeError: Cannot read property '0' of undefined
  at makeError (/Users/cloudnik/www/Automattic/core/plugins/gutenberg/node_modules/execa/lib/error.js:59:11)
  at handlePromise (/Users/cloudnik/www/Automattic/core/plugins/gutenberg/node_modules/execa/index.js:114:26)

The error is gone when the selector declaration is of form:

export const getEntityRecord = createSelector(
	< /* generics */ >( state, /* arguments */ ) => {},
	() => {}
);

But fails with the following forms:

export const getEntityRecord = createSelector(
	(< /* generics */ >( state, /* arguments */ ) => {}) as GetEntityRecord,
	() => {}
);
export const getEntityRecord = createSelector(
	getEntityRecordImplementation,
	() => {}
);

So we may need to revisit docgen once again.

@return

## What problem does this PR solve? Right now, the `createSelector` function loses the selector type details: ```ts const iReturnNumbers = createSelector( ( stringArg: string ) => 123, () => [] ); const iShouldBeANumber = iReturnNumbers(); // iShouldBeANumber is of type `any` ``` This is a problem in Gutenberg which depends on this package. For example, see the following PR: WordPress/gutenberg#41235 (comment) ## How does this PR propose to solve this problem? Adding the following `@return` type annotation preserves the selector type signature: ``` * @return {S} Memoized selector. ``` The above snippet now works as expected: ```ts const iReturnNumbers = createSelector( ( stringArg: string ) => 123, () => [] ); const iShouldBeANumber = iReturnNumbers(); // iShouldBeANumber is of type `number` ``` ## Test plan This changes just the documentation string, there are no runtime changes to test. Eyeballing the changes and confirming the types are now resolved as in the above examples should suffice. Caveat: this could be considered a breaking change for any projects using `createSelector` in TypeScript. After upgrading rememo to a patched version, the selectors will be of a different type, which could cause type errors. This could be addressed by releasing a new major point release. cc @aduth @dmsnell

adamziel · 2022-05-26T15:52:20Z

packages/core-data/src/selectors.ts

 }

-type RecordKey = number | string;
+type GenericRecordKey = number | string;


I retained this for now, but ideally we'll get rid of this type in a follow-up PR that will clean up the types in this file and add any missing signatures.

dmsnell · 2022-05-26T21:26:22Z

Concerning docgen I think we can try/catch on createSelector and return any if we don't get it. That's unfortunate, but I think I'm at the end of where I want to special-case that function.

Is the problem (a) that as Type alters the AST which we have hard-coded to match and (b) that passing a function as a value isn't a function expression in the AST?

adamziel · 2022-05-26T23:10:50Z

Is the problem (a) that as Type alters the AST which we have hard-coded to match and (b) that passing a function as a value isn't a function expression in the AST?

That’s my understanding, although I did not debug it

noisysocks · 2022-06-02T12:06:23Z

packages/core-data/src/selectors.ts

+		name: N,
+		key: KeyOf< K, N >,
+		query: EntityQuery< C >
+	): Partial< EntityRecordOf< K, N, C > > | null | undefined;


It would be nice if it were a compile time error to refer to a field that you didn't select.

const stuff = core.getEntityRecords( 'root', 'comment', { _fields: [ 'id', 'content' ] } ); console.log( stuff[ 0 ].title ); // shouldn't compile

Maybe this is possible by having the user specify (again...) which fields they are selecting as a type param.

const stuff = core.getEntityRecords< 'id' | 'content' >( 'root', 'comment', { _fields: [ 'id', 'content' ] } ); console.log( stuff[ 0 ].title ); // shouldn't compile

Yes it would! I hope to make it happen in one of the next iterations – let's keep this PR at atomic as possible.

packages/core-data/src/selectors.ts

packages/core-data/src/typed-rememo.ts

noisysocks · 2022-06-02T13:54:01Z

Very exciting and mind bending stuff! Thanks for running me through everything at WCEU.

I plopped this into the bottom of selectors.ts and TypeScript tells me that it thinks userView is of type User<'edit'> which ain't right – should be User<'view'>.

const userView = getEntityRecord( {} as State, 'root', 'user', 123, {
	context: 'view',
} );

…g the use of Omit<EntityQuery, '_fields'>

adamziel · 2022-06-02T23:28:14Z

@noisysocks Turns out it was caused by using Omit< EntityQuery, '_fields' > in one of the overloaded function signatures. I switched to a different way of matching a query with/without _fields and it seems to all work now!

adamziel · 2022-06-03T09:06:30Z

packages/core-data/src/selectors.ts

@@ -305,7 +365,12 @@ export const getEntityRecord = createSelector(
 		];
 	}
 );
+const commentDefault = getEntityRecord( {} as State, 'root', 'comment', 15 );


I left this accidentally, let’s remove it

noisysocks

LGTM once tests pass! 🙏

adamziel added [Type] Code Quality Issues or PRs that relate to code quality [Package] Core data /packages/core-data Developer Experience Ideas about improving block and theme developer experience labels May 23, 2022

adamziel requested a review from nerrad as a code owner May 23, 2022 11:52

adamziel self-assigned this May 23, 2022

adamziel commented May 23, 2022

View reviewed changes

adamziel mentioned this pull request May 23, 2022

TypeScript signatures for core-data selectors #39025

Closed

dmsnell reviewed May 24, 2022

View reviewed changes

packages/core-data/src/selectors.ts Outdated Show resolved Hide resolved

dmsnell reviewed May 24, 2022

View reviewed changes

adamziel mentioned this pull request May 26, 2022

Add return type to rememo jsDoc type annotations aduth/rememo#7

Closed

adamziel changed the title ~~Entity-aware type signature for getEntityRecord~~ Entity-aware type signature for getEntityRecord and getEntityRecords May 26, 2022

adamziel commented May 26, 2022

View reviewed changes

aduth mentioned this pull request May 27, 2022

Upgrade Rememo to v4 #41415

Merged

noisysocks reviewed Jun 2, 2022

View reviewed changes

packages/core-data/src/typed-rememo.ts Outdated Show resolved Hide resolved

adamziel added 5 commits June 2, 2022 14:04

Exhaustive type for getEntityRecord

78c8bce

Type signatures for getEntityRecord and getEntityRecords

d2d830e

Inline the implementation of getEntityRecordImplementation

80fb582

Explain the need for typed-rememo.ts

dce5397

Remove typed-rememo.ts

fc343d4

adamziel force-pushed the ts/get-entity-record-signature branch from ec6058a to fc343d4 Compare June 2, 2022 13:05

adamziel added 2 commits June 3, 2022 00:20

Use correct type inference for Context in getEntityRecords by removin…

eb36773

…g the use of Omit<EntityQuery, '_fields'>

Remove as any with as Kind[] and as Name[]

40c330e

adamziel requested review from ajitbohra and ryanwelcher as code owners June 2, 2022 23:23

adamziel requested review from juanmaguitar and fabiankaegy as code owners June 2, 2022 23:23

Clean up imports

a96e711

adamziel commented Jun 3, 2022

View reviewed changes

noisysocks approved these changes Jun 3, 2022

View reviewed changes

Remove dev artifacts

5d81376

adamziel merged commit 93cb857 into trunk Jun 4, 2022

adamziel deleted the ts/get-entity-record-signature branch June 4, 2022 07:15

github-actions bot added this to the Gutenberg 13.5 milestone Jun 4, 2022

adamziel mentioned this pull request Jun 8, 2022

[TypeScript] More specific core-data selector types #41594

Closed

adamziel mentioned this pull request Jul 7, 2022

[TypeScript] Simplify core-data types #42238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity-aware type signature for getEntityRecord and getEntityRecords #41235

Entity-aware type signature for getEntityRecord and getEntityRecords #41235

adamziel commented May 23, 2022 •

edited

Loading

adamziel May 23, 2022

dmsnell May 24, 2022

adamziel May 26, 2022

aduth May 27, 2022 •

edited

Loading

aduth May 27, 2022

adamziel May 28, 2022

mtias May 28, 2022

dmsnell May 24, 2022

adamziel May 25, 2022 •

edited

Loading

adamziel May 26, 2022

adamziel commented May 26, 2022 •

edited

Loading

adamziel May 26, 2022

dmsnell commented May 26, 2022

adamziel commented May 26, 2022

noisysocks Jun 2, 2022

adamziel Jun 2, 2022

noisysocks commented Jun 2, 2022

adamziel commented Jun 2, 2022

adamziel Jun 3, 2022

noisysocks left a comment

Entity-aware type signature for getEntityRecord and getEntityRecords #41235

Entity-aware type signature for getEntityRecord and getEntityRecords #41235

Conversation

adamziel commented May 23, 2022 • edited Loading

What problem does this PR solve?

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented May 26, 2022 • edited Loading

Choose a reason for hiding this comment

dmsnell commented May 26, 2022

adamziel commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks commented Jun 2, 2022

adamziel commented Jun 2, 2022

Choose a reason for hiding this comment

noisysocks left a comment

Choose a reason for hiding this comment

adamziel commented May 23, 2022 •

edited

Loading

aduth May 27, 2022 •

edited

Loading

adamziel May 25, 2022 •

edited

Loading

adamziel commented May 26, 2022 •

edited

Loading