Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Data: TypeScript definitions for entity records. #38666

Merged
merged 74 commits into from
Feb 22, 2022

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Feb 9, 2022

Description

This PR adds type definitions for default core data entity records:

export type EntityRecord< C extends Context > =
	| Attachment< C >
	| Comment< C >
	| MenuLocation< C >
	| NavMenu< C >
	| NavMenuItem< C >
	| NavigationArea< C >
	| Page< C >
	| Plugin< C >
	| Post< C >
	| Settings< C >
	| Sidebar< C >
	| Taxonomy< C >
	| Theme< C >
	| Type< C >
	| User< C >
	| Widget< C >
	| WidgetType< C >
	| WpTemplate< C >
	| WpTemplatePart< C >;

export type Updatable< T extends EntityRecord< any > > = {
	[ K in keyof T ]: T[ K ] extends RenderedText< any > ? string : T[ K ];
};

The fields that are only present in certain contexts are defined as in the JSON schema, for example:

export interface Taxonomy< C extends Context > {
	// ...
	/**
	 * A human-readable description of the taxonomy.
	 */
	description: ContextualField< string, 'view' | 'edit', C >;
	// ...
}

The locally edited versions of the records are wrapped with UpdatableRecord:

const post = {} as Post<'edit'>;
// post.content has type { raw: string; rendered: string; is_protected: boolean; block_version: string; }

const updatablePost = {} as Updatable< Post<'edit'> >;
// post.content has type string

This should allow us to build towards the following goals:

  1. Basic static type checks for entity-related core-data functions like getEntityRecord, getEntityRecords, select(), resolveSelect(), and useSelect()
  2. Basic static type checks for editing entity records. I always find it confusing when to use title.raw and title.rendered vs just the .title.
  3. API–context-aware static type checks. After fetching a Post with context: 'view', TS will know there's no such field as post.content.raw, but if you fetch the same Post with context: edit, post.content.raw will be a string.

The types are not published for now. When we do publish them, we'll have to restore the following README section on extending: https://gist.github.com/adamziel/ec41157c10ac11286609a635adb35a0b

Follow-up tasks:

  • Publish the types (the inverse of dde0888 )
  • Document the extending process (the inverse of dde0888 )
  • Implement TS signatures for core-data actions and selectors

cc @kevin940726 @dmsnell @jsnajdr @ntsekouras @mcsf @draganescu @noisysocks @talldan @tellthemachines @mkaz @gziolo

@adamziel adamziel added [Package] Core data /packages/core-data Needs Technical Feedback Needs testing from a developer perspective. Developer Experience Ideas about improving block and theme developer experience labels Feb 9, 2022
@adamziel adamziel requested a review from nerrad as a code owner February 9, 2022 12:49
@adamziel adamziel self-assigned this Feb 9, 2022
@adamziel adamziel changed the title Add type definitions for core data entity records TypeScript definitions for core-data entity records Feb 9, 2022
@dmsnell
Copy link
Member

dmsnell commented Feb 9, 2022

Thanks for posting this @adamziel. Here are a few quick thoughts I had while skimming:

  • it would be easier for me to read if we had a separate types file for each entity kind. having 1200 lines to scour to find the Theme type is a bit overwhelming
  • seeing date?: string | null; reads awkwardly to me and I wonder if that's the real type or if it's something like date?: string; or date: string | null;
  • when we're making every property optional it's a bit awkward as well, but if we do that, we can define it with the actual properties we expect and wrap the thing in Partial<…> which gives us a semantic step to better understand the object.

The types address the problem of different contexts (edit, view, and embed) by making every property optional (date?:) – should it be? Perhaps we could have specialized types for each context like Attachment or Attachment, and give them stronger guarantees?

Can you clarify what you mean here by edit, view, and embed in context of things like attachment or theme objects?

If they are generalizable it'd likely work out well to have the other direction:

type EditableEntity<Entity> = Entity;
type ViewableEntity<Entity> = Partial<Entity>;

but I don't feel like I understand exactly what you are wanting to do and what needs we have. as a reminder, there is no real typing without a purpose for the typing, so as you mention here, what we want and need in different contexts is going to lead the design of the types.

Edited records use title: string instead of title: { raw: string, rendered: string; } so that's another special case to handle

Are you talking about the fact that we might store a string in the app but the API response has a different version of the data? If that's the case we may find that we have a different type for the API response and the internal data. That's a good thing though! By the way we can handle these cases through type utilities if the case is general enough and warranted.

Post, Page, have a lot in common – perhaps they could have a common abstraction.

it's worth a look but my experience tells me the duplicate code would be worth it. they will likely diverge in their evolution and be used in different ways or places. if they have enough overlap we might discover that there's only a single type, but likely two is fine.

We could extract common types like "publish" | "draft" | ... enum or RawString = { raw: string, rendered: string; }.

👍 to this. I generally try to avoid type unions inside of record definitions

// not this
type Alert = {
	criticality: 'high' | 'low';
	ttl: number;
}

// but this
type Criticality = 'high' | 'low';
type Alert = {
	criticality: Criticality;
	ttl: number;
}

How often do these schemas change? Do we need an automation to confirm these types are still accurate, or is it okay to leave updating to developers when they need to use a new API feature?

API response types are inherently a lie. we can't rely on them so they are more or less a heuristic tool for developers to discover properties they expect. they are going to represent a safety hole in the system though if we cast an API response to these types.

the only way to provide a durable and safe type is to only create it after parsing an API response where a failure to adhere to the expectations produces a parse failure instead of an invalidly-typed object. that's not likely to be useful here.

all that is to say as far as maintenance goes I'd bet we're fine leaving these and letting people fix or change them as WordPress changes. remember that plugins might alter these API responses as well so really we can't say anything for sure about them. given that uncertainty they can be helpful for what they are: reasonable guides.

What resolution is the best here? We can cover every possible data shape at the cost of higher maintenance burden, or we could replace complex nested definitions with Object at the cost of occasional mistakes due to less static checks

Same as the last thing I mentioned. We can type everything unknown and then we're good 😄
Seriously though this is why typing API responses is bad enough let alone when introducing them into a system with different modifications of that data.

What is the primary goal of adding these types?
What do you want to accomplish by having them?
How do you see developers using these types?

@gziolo
Copy link
Member

gziolo commented Feb 9, 2022

I only wanted to link the related tracking issue #18838.

@adamziel
Copy link
Contributor Author

adamziel commented Feb 10, 2022

  • it would be easier for me to read if we had a separate types file for each entity kind. having 1200 lines to scour to find the Theme type is a bit overwhelming

Good call @dmsnell! I just updated this PR

What is the primary goal of adding these types? What do you want to accomplish by having them? How do you see developers using these types?

I don't have laser focused requirements. The ultimate goal is to enjoy the usual benefits of static analysis across Gutenberg, and core-data entities are important building blocks.

That being said, it would already be quite useful with useSelect to help with common mistakes like:

function PostTitle({ postId }) {
	// TypeScript understands that post is a Post or undefined
	const post = useSelect( select => select( coreStore ).getEntityRecord(
		'postType',
		'post',
		postId,
		{ context: 'view' }
	), [ postId ]);
	
	return (
		<div>
			{/* TS warns me post may still be undefined */}
			{post.title.rendered}
			{/* TS warns me I have the property name wrong */}
			{post.name}
			{/* Brownie points: TS warns me this property is not available in the view context */}
			{post.title.raw}
		</div>
	);
}

Can you clarify what you mean here by edit, view, and embed in context of things like attachment or theme objects?

WordPress REST API returns different subsets of field depending on the context query parameter. For example, looking at the attachment JSON schema I got with the following CURL command:

curl -XOPTIONS http://localhost:8888/wp-json/wp/v2/media/

We see that schema contains the following field:

{
      "date": {
        "description": "The date the post was published, in the site's timezone.",
        "type": [ "string", "null" ],
        "format": "date-time",
        "context": [ "view", "edit", "embed" ]
      }
}

This field will be exposed only for requests with ?context=view, ?context=edit, or ?context=embed. If context is missing, the API usually assumes view. I believe the API only allows contexts that are defined in at least one of the fields.

Now, here's a different field:

{
    "description": {
      "description": "The attachment description.",
      "type": "object",
      "context": [
        "view",
        "edit"
      ]
}

It won't be returned by the API if I send a request with ?context=embed.

  • seeing date?: string | null; reads awkwardly to me and I wonder if that's the real type or if it's something like date?: string; or date: string | null;

Good spot, it looks weird to me, too! Based on the underlying JSON schema I quoted earlier, it may be either a string or a null, but it shows up in all the contexts supported by the media endpoint so we can just say date: instead of date?:

  • when we're making every property optional it's a bit awkward as well, but if we do that, we can define it with the actual properties we expect and wrap the thing in Partial<…> which gives us a semantic step to better understand the object.

It is awkward indeed. Partial<...> could be the way, but I'd like to see if we can do without it. The first step there would be removing the optionality from properties present in all supported contexts.

Edited records use title: string instead of title: { raw: string, rendered: string; } so that's another special case to handle

Are you talking about the fact that we might store a string in the app but the API response has a different version of the data? If that's the case we may find that we have a different type for the API response and the internal data. That's a good thing though! By the way we can handle these cases through type utilities if the case is general enough and warranted.

Ah, I meant a quirk of how core-data treats certain properties. For example, posts retrieved using getEntityRecord have a title as an object with raw and rendered properties with raw block markup, and its rendered version respectively. If you start editing the data, though, and use getEditedEntityRecord, the title becomes a string containing the edits. These special properties are listed in rawAttributes in entities.js

Post, Page, have a lot in common – perhaps they could have a common abstraction.

it's worth a look but my experience tells me the duplicate code would be worth it. they will likely diverge in their evolution and be used in different ways or places. if they have enough overlap we might discover that there's only a single type, but likely two is fine.

👍

API response types are inherently a lie. we can't rely on them so they are more or less a heuristic tool for developers to discover properties they expect. they are going to represent a safety hole in the system though if we cast an API response to these types.

Just to confirm I understand - you are saying that casting the actual data type received from the API to the one declared by the API represents a safety hole, correct?

all that is to say as far as maintenance goes I'd bet we're fine leaving these and letting people fix or change them as WordPress changes. remember that plugins might alter these API responses as well so really we can't say anything for sure about them. given that uncertainty they can be helpful for what they are: reasonable guides.

👍

@adamziel
Copy link
Contributor Author

adamziel commented Feb 10, 2022

Spontaneous idea: To handle contexts, we could define "full" types with all the properties, and then derive context-based types using `Pick:

interface Attachment {
// ... lots of fields
}
// Let's find a better name :-)
type AttachmentInViewContext = Pick<Attachment, 'date' | 'guid' | ...>;

That doesn't solve the varying shape of "raw" fields like guid and title, though. Potentially it could be more like this:

interface RawFieldInViewContext {
    rendered: string;
}
interface RawFieldInEditContext {
    raw: string;
    rendered: string;
}

interface Attachment {
// ... lots of fields
}
interface AttachmentInViewContext extends Pick<Attachment, 'date' |  ...> {
    guid: RawFieldInViewContext;
}
interface AttachmentInEditContext extends Pick<Attachment, 'date' |  ...> {
    guid: RawFieldInEditContext;
}

It's getting pretty complex, though. Maybe the returns are diminishing at this point? We could move forward with the optional fields and iterate if needed.

@adamziel
Copy link
Contributor Author

adamziel commented Feb 10, 2022

@johnbillion
Copy link
Member

Related issue for wp-types: johnbillion/wp-json-schemas#40

@adamziel
Copy link
Contributor Author

I added some types for raw data fields:

type Post< RawObject > // title is an object like {raw: "", rendered: ""}
type Post< RawString > // title is a string

I'm still noodling on a different naming scheme like ServerEntity< Post > and EditedEntity< Post >, but nothing blocking. I think this one is ready for another round of feedback cc @gziolo @kevin940726 @dmsnell @youknowriad

@adamziel
Copy link
Contributor Author

adamziel commented Feb 10, 2022

I just had this idea for entity types with context-specific fields:

interface BaseAttachment {
   // ... lots of fields ... 
}

type ViewContext = 'ViewContext';
type EditContext = 'EditContext';
type EmbedContext = 'EmbedContext';
type EntityContext = ViewContext | EditContext | EmbedContext;

type ViewProperties = 'date' | 'date_gmt';
type EditProperties = 'date' | 'id';

export type Attachment< Context extends EntityContext > = Pick<
	BaseAttachment,
	Context extends ViewContext ? ViewProperties : EditProperties
>;

// In another file:
const photo : Attachment< EditContext > = getEntityRecord( 'root', 'media', 15 );

It would require quite a bit of refactoring, though, so I'll wait for feedback before acting on this.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attachment< EditContext >

This feels inverted to me. I don't think we'll really want to inject the context into the type and then have the type decide what it is based on that context.

Your code snippet looks fine but maybe we could consider instead of Attachment<Context> and BaseAttachment something like Attachment and AttachmentInContent<EditContext>

Really I'd rather personally be dealing with EditContext<Attachment> but I can see where it would be hard to create the typings for that, would need to couple all the different types together, unless we made those inside each module.

// attachment.ts

export interface Attachment  {}

export type InContext<Context extends AttachmentContext> =
	Context extends EditContext ? Pick<Attachment, EditFields> :
	Context extends ViewContext ? Pick<Attachyment, ViewFields> :
	never;
import * as Attachment from '…/attachment';
import { EditContext } from '…/entity-contexts…';

const attachment: Attachment.InContext<EditContext> = 

Just thoughts. I wish we could have it all ways and I'm not sure what's best here, just that something about seeing Attachment<EditContext> worries me about muddying our interfaces.

@kevin940726
Copy link
Member

kevin940726 commented Feb 11, 2022

First of all, I think this will be a really big scale and can be hard to tackle, so thank you for starting this!

I agree that the optional field is awkward (date?: string). Instead, I'd prefer to only use optional fields sparingly, if the field should be there then it should be there. If the users really want to mark them optional they can always wrap it in Partial<> themselves.

As for keeping the types in sync with the API, I have no knowledge of how the types are generated and maintained on the PHP side, but I'd imagine a workflow to also type check the PHP response (either in runtime tests or static type check) so that we can make sure the schema is always valid. Then, we can repeat what @adamziel have done here, generate the typescript definition files via a script automatically whenever those schema change. Not sure how doable it is though, and might be a good candidate for another PR rather than this one. One challenge here might be that the code of the REST API is in both repo (gutenberg and core), not sure what to do with it (another benefit to having a monorepo 😆 ?).

I just had this idea for entity types with context-specific fields:

This might also be the simplest method I can think of too. We can create the Context type as an enum and share it across different files. And instead of 'ViewContext', we can just call it 'view' to match the schema. I think it also makes sense to make it a generic since that context will default to view and we can just do Attachment for that case.

Note that we probably won't need to cast it ourselves anyway, useEntityRecord or other hooks should be able to interpret the value of context and automatically return the desired type.

@adamziel
Copy link
Contributor Author

adamziel commented Feb 22, 2022

All the checks are green – this could be it 😮

Copy link
Contributor

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, exciting, wonderful! I don't have enough positive adjectives for how exciting it is that this is working 🎉

I don't think we should publish these types though, unless my understanding of how this package is used is incorrect. As long as I understand things correctly, to be able to publish these, we'll need to at minimum be able to implement the additional interfaces to the @wordpress/data types like happen in the DefinitelyTyped types here:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/wordpress__core-data/index.d.ts

packages/core-data/package.json Outdated Show resolved Hide resolved
packages/core-data/src/types/README.md Show resolved Hide resolved
packages/core-data/src/types/wp-base-types.ts Outdated Show resolved Hide resolved
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a typo but I'm really happy with all these. Well done 👏

packages/core-data/src/types/base-entity-types.ts Outdated Show resolved Hide resolved
* const c : Comment< 'view' > = ...;
*
* // c.numberOfViews is a number
* // c.id is still present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this all so much ❤️

@adamziel adamziel merged commit 87a7fbe into trunk Feb 22, 2022
@adamziel adamziel deleted the ts/add-core-data-types branch February 22, 2022 23:41
@github-actions github-actions bot added this to the Gutenberg 12.7 milestone Feb 22, 2022
@gziolo
Copy link
Member

gziolo commented Feb 23, 2022

Impressive work everyone 👏🏻

@cbravobernal cbravobernal added the [Type] Code Quality Issues or PRs that relate to code quality label Feb 27, 2022
@cbravobernal cbravobernal changed the title TypeScript definitions for core-data entity records Core Data: TypeScript definitions for entity records. Feb 27, 2022
dmsnell added a commit that referenced this pull request Mar 4, 2022
When we introduced the types for the core entities in #38666 we used a
directory named `types`. Unfortunately this meant that we can't create the
customary file `src/types.ts` because of the name clash in module resolution.

In this patch we're renaming the directory so that we can separately
import `entity-types(/index.ts)` and also `types(.ts)` without the
naming conflict.
adamziel pushed a commit that referenced this pull request Mar 7, 2022
When we introduced the types for the core entities in #38666 we used a
directory named `types`. Unfortunately this meant that we can't create the
customary file `src/types.ts` because of the name clash in module resolution.

In this patch we're renaming the directory so that we can separately
import `entity-types(/index.ts)` and also `types(.ts)` without the
naming conflict.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience Needs Technical Feedback Needs testing from a developer perspective. [Package] Core data /packages/core-data [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants