Simplify TypeScript types in wordpress/core-data #43515
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamziel
requested review from
nerrad,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +552 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
simplify/core-data-ts
branch
from
24d999a
to
37d8a68
Compare
adamziel
added
[Type] Enhancement
gziolo
reviewed
Aug 24, 2022
gziolo
approved these changes
Aug 24, 2022
There was a problem hiding this comment.
I finished checking the code. I don't know all the implementation details of @wordpress/core-data and I'm not an expert in TypeScript, but the changes proposed to simplify the code. It definitely makes it easier to follow the implementation now that types are more concise.
I think it's a reasonable trade-off to write more verbose code like:
getEntityRecords< WpTemplate >( 'postType', 'wp_template', 123 );
getEditedEntityRecord< WpTemplate >( 'postType', 'wp_template', 123 );In fact, it's probably the best way to tackle it anyway, as it removes all possible mistakes when inferring types from params passed. In exchange, we won't have to maintain a complex logic that powered that behavior.
It would be ideal to have more folks reviewing this PR, but it's also fine to land it in a week or two and iterate later. I'm sure we will need more iterations to finalize all those types.
By the way, I love the part that allows to extend the list of supported entity types. Excellent solution that makes it possible to cover custom REST API endpoints by plugin authors ❤️
|
|
||
| const POST_RAW_ATTRIBUTES = [ 'title', 'excerpt', 'content' ]; | ||
|
|
||
| export const rootEntitiesConfig = [ |
There was a problem hiding this comment.
An unrelated note to the refactoring in this PR:
What's the relation between root entities and types defined for entities? I would be curious to learn whether there is one to one relationship and how can I check whether all endpoints have types defined.
There was a problem hiding this comment.
All root entities can have a related entity type. Most already do. The few missing ones can be easily backfilled.
| @@ -13,7 +13,6 @@ import type { | |||
| } from './helpers'; | |||
|
|
|||
| import type { BaseEntityRecords as _BaseEntityRecords } from './base-entity-records'; | |||
| import type { DefaultContextOf } from './index'; | |||
|
|
|||
| declare module './base-entity-records' { | |||
There was a problem hiding this comment.
Unrelated to this PR:
How can I identify which REST API endpoint returns this entity type? It would be nice to include a comment with references to REST API docs for all entity types defined.
There was a problem hiding this comment.
Oh yes, that's a good idea!
| * export interface PerPackageEntities< C extends Context > { | ||
| * myPlugin: OrderEntity | ClientEntity | ||
| * export interface PerPackageEntityRecords< C extends Context > { | ||
| * myPlugin: Client | Order<C>> |
There was a problem hiding this comment.
Is it a typo in Order<C>> - double greater than sign?
There was a problem hiding this comment.
I'll surf the wave of green checks in this PR and fix it in a follow-up
|
I think this is the right direction 😀 there's a happy medium when it comes to typing things in TypeScript given the compiler's limitations. Can I please have that hour of my life back? (just kidding, I could listen to you for days) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Replaces very complex core-data selectors signatures introduced in #39025 with lightweight ones.
After this PR, the consumer has to pass the return type like
getEntityRecord<Post>( 'root', 'postType', 15 )orgetEntityRecord/** @type {Page} */( 'root', 'postType', 15 ). It adds a little bit of work, but makes the types reliable and the signatures succinct.Before this PR, the
getEntityRecord( 'root', 'postType', 15 )call inferred the correct return type. That's handy for the consumer, but the implementation is so mind-boggling that it took more than an hour for me to explain it to @noisysocks. Also, there's so many layers that TypeScript makes consuming these types hard. Do something too complex and TypeScript will gives up and replace them withanyas seen in #41578.Why?
I want to publish the TypeScript types for the
core-datapackage and start with something simple. This PR offers a set of types that's easy to improve, change, and iterate on.Testing Instructions
Confirm the checks are green
cc @gziolo