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: Draft a type for an Entity's config paramater #39481

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions packages/core-data/src/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
export type Context = 'edit' | 'embed' | 'view';

interface Edit {}

// @ts-ignore
export interface EntityRecord< C extends Context > {}

export interface EntityConfig< Record extends EntityRecord<any> > {
Copy link
Contributor

Choose a reason for hiding this comment

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

What else can we call it? This name conflicts with the built-in type Record. Perhaps R wouldn't be too terrible? Or _Record?

Copy link
Contributor

@adamziel adamziel Mar 17, 2022

Choose a reason for hiding this comment

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

I am not sure if parametrizing this will play along with as const/DeepReadOnly and the lookup table approach. If not, perhaps we could hardcode the key and the context in TypeScript, separately from the config, and then constrain the EntityConfig type to only express these hardcoded values? But that seems backwards, as if the type was the actual config.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure what I was thinking there with Record. nevertheless, we should consider all of this possibly junk-code because I was laser-focused on the other aspects while writing it out.

/** Path in WP REST API from which to request records of this entity. */
baseURL: string;

/** Arguments to supply by default to API requests for records of this entity. */
baseURLParams: { context: Context };
Copy link
Contributor

@adamziel adamziel Mar 16, 2022

Choose a reason for hiding this comment

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

The type here is more of

Suggested change
baseURLParams: { context: Context };
baseURLParams?: Record<string, string> & { context?: Context }


/**
* Returns the title for a given record of this entity.
*
* Some entities have an associated title, such as the name of a
* particular template part ("full width") or of a menu ("main nav").
*/
getTitle( record: Record ): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be null

Suggested change
getTitle( record: Record ): string;
getTitle( record: Record )?: string;


/**
* Indicates an alternate field in record that can be used for identification.
*
* e.g. a post has an id but may also be uniquely identified by its `slug`
*/
key: string;

/**
* Collection in which to classify records of this entity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another word would be a "namespace", since some frameworks refer to database tables as "collections". Namespace isn't perfect too, though – I guess we could use either.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah it still puzzles me and I think the reason is that this kind value connotes different things at the same time. I'm reluctant to use the term namespace because I think that's a back-justification. one of the effects is namespacing, but it's really not used that way.

  • namespace is used as part of the API to get versions of the resources, e.g. wp/v2 or wpcom/v1.1.
  • it's clearly being used as categorization or classification when contrasting postType and taxonomy. in cases such as in calling getEntityRecord it definitely feels more like "this general kind of entity record" as a grouping term rather than "the entity records provided by this plugin" which is generally how we use "namespace" elsewhere in the project

probably what's throwing us both off is that getOrLoadEntitiesConfig was built upon kind when in fact that function serves a subtly different purpose; more again an incidental design element related to optimization rather than intent.

*
* 'root' is a special name given to the core entities provided by the editor.
*
* It may be the case that we request an entity record for which we have no
* valid config in memory. In these cases the editor will look for a loader
* function to requests more entity configs from the server for the given
* "kind." This is how WordPress defers loading of template entity configs.
*/
kind: string;

/** Translated form of human-recognizable name or reference to records of this entity. */
label: string;

/** @TODO: What is this? */
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question! I don't remember, we'll have to find out.

mergedEdits: {
meta?: boolean;
}

/** Name given to records of this entity, e.g. 'media', 'postType', 'widget' */
name: string;

/**
* Manually provided plural form of the entity name.
*
* When not supplied the editor will attempt to auto-generate a plural form.
*/
plural: string;

/**
* Fields in record of this entity which may appear as a compound object with
* a source value (`raw`) as well as a processed value (`rendered`).
*
* e.g. a post's `content` in the edit context contains the raw value stored
* in the database as well as the rendered version with shortcodes replaced,
* content texturized, blocks transformed, etc…
*/
rawAttributes: (keyof Record)[];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is optional and assumed to be an empty list if missing. Also, I don't think it can be anything else than a string.

Suggested change
rawAttributes: (keyof Record)[];
rawAttributes?: string[];

Copy link
Member Author

Choose a reason for hiding this comment

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

that's fine, keyof Record should be a string but this constraint mandates that it's a field we actually define in the Record type. not relevant if we don't parameterize by record, but I've added in here at least for thinking about this type because clearly the config value is in effect parameterized by the record type.

if we remove that inherent coupling we don't sacrifice much, but for example, we miss out on guidance in getTitle.

removing that coupling is a real option. like much of this project these properties tend to be added over time as need arises and so they're not necessarily orchestrated as a unified whole. if practical, I think it would be great to keep it because then you would be able to write getTitle and have it show you the available fields in the record that you could use.


/**
* Which transient edit operations records of this entity support.
*/
transientEdits: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is optional as well

Suggested change
transientEdits: {
transientEdits?: {

blocks?: boolean;
selection?: boolean;
}

// Unstable properties

/** Returns additional changes before applying edits to a record of this entity. */
__unstablePrePersist( record: Record, edits: Edit[] ): Edit[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__unstablePrePersist( record: Record, edits: Edit[] ): Edit[];
__unstablePrePersist( record: Record, edits: Edit[] )?: Edit[];


/** @TODO: What is this? Used in `canEdit()` */
__unstable_rest_base: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this is at the moment, but it's optional

Suggested change
__unstable_rest_base: string;
__unstable_rest_base?: string;

}