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

[Consistency] Use the word "record" for the entity record TypeScript types #39251

Merged
merged 4 commits into from
Mar 14, 2022

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Mar 7, 2022

Part of #39025

Description

#38666 introduced a set of TypeScript types to model entity records data. Unfortunately, these interfaces are sometimes referred to as "entity types" and some other times as "entity records". I'd like to be as precise as possible, since core-data already uses a lot of different words to name the same ideas. I recommend placing the base record types in the BaseEntityRecords namespace instead of the BaseEntityTypes namespace.

Furthermore, since there is quite a few related files, this PR moves them into a subdirectory called records to make the contents of the entity-types directory more readable, especially when additional types like Kind and Name get merged in #39025.

Testing Instructions

Audit the types. As a type-only change this won't impact the built files.
Furthermore these types have been exported by @wordpress/core-data; that
shouldn't change (verify please 😄 )

Types of changes

This is a type-only change and won't impact the build.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@adamziel adamziel added [Type] Code Quality Issues or PRs that relate to code quality [Package] Core data /packages/core-data labels Mar 7, 2022
@adamziel adamziel requested a review from nerrad as a code owner March 7, 2022 12:35
@adamziel adamziel self-assigned this Mar 7, 2022
@adamziel adamziel changed the title Rename "Entity type" interfaces to "Entity records" [Consistency] Use the word "record" for the entity record TypeScript types Mar 7, 2022
@dmsnell
Copy link
Member

dmsnell commented Mar 7, 2022

An entity type refers to the overarching concept like a Comment, and an entity record refers to a specific instance of it, such as a comment with id 149.

If I read this then I would assume the TypeScript type for a comment record would be type and not record. This is akin to any type/value splits: number is a type and 13 is a value of type number. Is a comment record a thing of type Record that somehow relates back to a comment Type or is a comment record a value of the type Comment(Type)?

I'm noting that I still import the …Record types from entity-types even though they are defined in entity-types/records

@adamziel
Copy link
Contributor Author

adamziel commented Mar 8, 2022

@dmsnell to surface the takeaways from our chat:

  • The "Entity" seems to be the overarching concept, like a "Comment". Perhaps like a metaclass or a database table?
  • The "Entity Config" contain the data such as the label and baseURL to fetch the data from, but also kind and name.
  • The "Entity Record" represents a single database row, sort of like in the Active Record pattern, or an object to be saved with an object mapper.

It doesn't seem like an "Entity type" is a thing.

All of that being said, I think this PR may be good to go as it is right now.

It would be great to update the terminology used across core-data to refer to all these concepts, but that's outside of scope of this PR.

I'm noting that I still import the …Record types from entity-types even though they are defined in entity-types/records

I was thinking of the entity-types (the directory name) as of a place where the TypeScript types for the entities live, unrelated to the idea of an Entity Type. It seems like we can ditch the term Entity Type entirely.

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.

The "Entity" seems to be the overarching concept, like a "Comment". Perhaps like a metaclass or a database table?

Not just the concept, but the apparatus that governs fetching, loading, storage, and updates. Entity is more like a Manager than a concept.

That's the only amendment I'd propose to your summary of our chat.

I was thinking of the entity-types (the directory name) as of a place where the TypeScript types for the entities live, unrelated to the idea of an Entity Type

It actually leaves open the opportunity to talk about EntityType as a new thing. Given that entityType doesn't currently exist I think we can head-off some confusion and use it to refer to the type of a record for a given entity.

That would imply removing the entity-types/record subdirectory as redundant, maybe moving the helpers and utility and base types into src/types.ts or maybe not, just leaving them inside entity-types.

Point is I don't think we need entity-types/records/comment.ts instead of entity-types/comment.ts.

Whaddyathink? Everything else in this PR is good and necessary to avoid type and lint issues. I'm approving even though I think that records subdirectory only adds indirection where none is needed, but I'm putting that into your hand to decide. Feel no pressure to merge, also none to delay.

@adamziel
Copy link
Contributor Author

The "Entity" seems to be the overarching concept, like a "Comment". Perhaps like a metaclass or a database table?

Not just the concept, but the apparatus that governs fetching, loading, storage, and updates. Entity is more like a Manager than a concept.
That's the only amendment I'd propose to your summary of our chat.

👍

I started a PR to introduce a consistent set of terms for things, let's keep the discussion going there: #39349

Point is I don't think we need entity-types/records/comment.ts instead of entity-types/comment.ts.

Whaddyathink? Everything else in this PR is good and necessary to avoid type and lint issues. I'm approving even though I think that records subdirectory only adds indirection where none is needed, but I'm putting that into your hand to decide. Feel no pressure to merge, also none to delay.

Sounds fair. I created a new subdirectory so that it wouldn't get in a way of finding files such as selectors.ts and actions.ts, but I kind of forgot that defining selector types separately is just a temporary workaround until core-data becomes typescript-compliant at which point we won't need them anymore. Cool. I moved all these types to entity-types/ 👍

@adamziel adamziel force-pushed the ts/refactor-entity-types-to-entity-records branch from f894e34 to b7917af Compare March 10, 2022 14:15
@adamziel adamziel force-pushed the ts/refactor-entity-types-to-entity-records branch from d581633 to f803ae4 Compare March 11, 2022 09:05
@adamziel adamziel merged commit e6ff52c into trunk Mar 14, 2022
@adamziel adamziel deleted the ts/refactor-entity-types-to-entity-records branch March 14, 2022 12:13
@github-actions github-actions bot added this to the Gutenberg 12.9 milestone Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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.

2 participants