Initial entities implementation #6462

youknowriad · 2018-04-27T07:36:39Z

This PR tries to implement the idea of an entities abstraction. An entity is an abstraction of a WP entity (post, taxonomy, postType, category...). It allows to automatically generate selectors, resolvers for the said entity in the core data module.

In this PR, it's a basic implementation:

Entities are static (could be enhanced to be made dynamic)
It supports only fetching by PK (could be easily extended to add more selectors)
I'm using it for the postType and media entities at the moment.

This PR is blocking #6384 and is related to #6395

jorgefilipecosta · 2018-04-28T18:08:05Z

core-data/resolvers.js

-	yield receivePostTypes( postType );
+export async function* getModelRecord( state, kind, name, primaryKey ) {
+	const modelConfig = getModel( kind, name );
+	const record = await apiRequest( { path: `${ modelConfig.baseUrl }/${ primaryKey }?context=edit` } );


Maybe we should allow model to configure the request as some models probably don't require context=edit.

I tend to think we should always use the edit context (or remove the context concept entirely) but yeah let's keep this way and see if we need to make it configurable as we add models.

I'd also like to remove it entirely. At least from this abstraction.

jorgefilipecosta · 2018-04-28T18:17:50Z

Regarding the functional point of view, these changes test well, awesome work 👍
I like this introduction of the model's concept as it could allow us to reduce duplicate code in actions, resolvers, and selectors.
I'm curious about the future plans. Is the idea to involve this model concept? E.g: allow us to mutate and change models besides just retrieving requests? Should we allow the automatic generation of the general request that requests all records e.g: getPostTypes() that requests '/wp/v2/types/', (with opt-in or opt-out flag in the model to enable or disable this). This would allow us to not repeat requests if the "general request" was executed before and now we want a specific record that was already retrieved in the "general request".

youknowriad · 2018-04-30T08:33:53Z

I'm curious about the future plans. Is the idea to involve this model concept? E.g: allow us to mutate and change models besides just retrieving requests?

Yes exactly, mutation, pagination, query fetching...

This would allow us to not repeat requests if the "general request" was executed before and now we want a specific record that was already retrieved in the "general request".

General request => yes, this just sets the baseline but I don't think we should use it to retrieve a specific record because of pagination.

gziolo · 2018-04-30T08:42:41Z

core-data/reducer.js

-			};
-	}
+function model( modelConfig ) {
+	return ( state = { byPK: {} }, action ) => {


Can we avoid using PK abbreviation and use consistently PrimaryKey? :D

gziolo · 2018-04-30T08:52:54Z

core-data/models.js

+import { find } from 'lodash';
+
+const models = [
+	{ name: 'postType', kind: 'root', pk: 'slug', baseUrl: '/wp/v2/types' },


It should be type rather than kind for consistency. I guess this is because of the fact that type is a reserved word in actions, right?

Yes it's one of the reasons. Also, type is overused :) so we could have a postype of type type :)

Can we have a post type of kind type? :D

gziolo · 2018-04-30T08:55:09Z

This is cool 👍

youknowriad · 2018-04-30T09:11:41Z

How can we move forward with this, I'd love to merge it to unblock #6384

youknowriad · 2018-04-30T18:45:56Z

@aduth I'd appreciate your thoughts on this one

aduth · 2018-04-30T20:39:39Z

and is related to #6395

Can you elaborate on in what way this relates to #6395 ? Do they complement or conflict with one another?

Am planning to take a closer look this evening.

aduth · 2018-04-30T20:41:39Z

What exactly is kind ?

aduth · 2018-04-30T20:42:41Z

core-data/reducer.js

 }

+const modelsByKind = groupBy( modelsConfig, 'kind' );
+export const models = combineReducers( Object.entries( modelsByKind ).reduce( ( memo, [ kind, subModels ] ) => {


This whole sequence of logic could do for some explaining variables, newlines, comments. Quite difficult to interpret.

aduth · 2018-04-30T20:45:28Z

core-data/selectors.js

 */
-export function getPostType( state, slug ) {


I assume we have some using the existing selector to be updated (work-in-progress) ?

This PR now generates dynamically the selector in core-data/model-selectors.js. So everything keeps working as before :)

aduth · 2018-04-30T20:47:53Z

core-data/models.js

+	{ name: 'postType', kind: 'root', primaryKey: 'slug', baseUrl: '/wp/v2/types' },
+];
+
+export function getModel( kind, name ) {


I think you'd at one point floated the idea to me that this configuration could live in state, which would enable others to more easily extend with their own models, and might make it align better with existing concepts like getting via selector, not the standalone utility function here.

Yes true, I didn't want to build a huge PR directly on the first iteration, I do think we need dynamic models, I think it's needed in #6384 but was thinking of adding those in another iteration.

aduth · 2018-04-30T20:48:58Z

core-data/models.js

+import { find } from 'lodash';
+
+const models = [
+	{ name: 'postType', kind: 'root', primaryKey: 'slug', baseUrl: '/wp/v2/types' },


I don't know that it's an issue, but I could foresee some confusion on primaryKey naming with how it might be confused or misinterpreted as the database concept, where in the case of post types there is no database entity, and for others we might choose a key which isn't actually the primary key for the database entity. Maybe just me?

I don't know, I've always used primaryKey personally but I can update to identifier if you think it's better.

Maybe just key ? Or keyBy (#) ?

aduth · 2018-04-30T20:49:30Z

core-data/model-resolvers.js

@@ -0,0 +1,19 @@
+/**


Should we organize models-related files into their own folder if there's several related ones which stand apart from the rest of the files?

Move those to the index.js. It's not easy to find a good location without creating cycle dependencies.

aduth · 2018-04-30T20:50:59Z

core-data/model-resolvers.js

+import modelsConfig from './models';
+import { getModelRecord } from './resolvers';
+
+export default modelsConfig.reduce( ( memo, { kind, name } ) => {


Minor: Just noting that this is a runtime evaluation which could theoretically be pre-evaluated (the sort of thing prepack would love to optimize for).

Haha, are you suggesting we try prepack :) I haven't played with it yet

Haha, are you suggesting we try prepack :)

Nah, not necessarily. Just remarking that it's wasted work that could be pregenerated.

There's also this, which might be more of a viable option if it's really a concern: https://github.com/kentcdodds/babel-plugin-preval

aduth · 2018-04-30T20:52:03Z

core-data/reducer.js

-			};
-	}
+function model( modelConfig ) {
+	return ( state = { byPrimaryKey: {} }, action ) => {


What other properties do you foresee living in this state object?

I'm thinking queries maybe, but it could be added later, I can update this PR to remove this key at the moment.

youknowriad · 2018-05-01T09:29:19Z

What exactly is kind ?

Kind is just a way to group models to avoid duplication, I see these as possible values: root, postType, taxonomy.

This will avoid ambiguity in the reducers and at the selectors level.

How it relates to #6395

I believe there a conflict in the implementation details but they share similar goes. Instead of having helpers to create reducers/selectors/actions... the models abstraction generate them. #6395 adds the queries data though, and I think this could be refactored to be included in the models abstraction. (each model will generate its queries sub state and its selectors).

gziolo · 2018-05-02T09:20:55Z

core-data/resolvers.js

-	const postType = await apiRequest( { path: `/wp/v2/types/${ slug }?context=edit` } );
-	yield receivePostTypes( postType );
+export async function* getEntityRecord( state, kind, name, key ) {
+	const entity = getEntity( kind, name );


What does entity contain, is it config? It looks like it. Should we rename to:

const entityConfig = getEntityConfig( kind, name );

to make it easier to follow?

If we use entityConfig it means we need an entity too, but we don't have such a thing.

I think it's just entity. I've used entityConfig in the reducer to avoid variable shadowing.

youknowriad · 2018-05-02T09:22:54Z

In the last commit 9609b27 I've updated the media to use the abstraction.

youknowriad · 2018-05-02T09:23:32Z

I feel it's pretty solid for a V1, let me know how to move this forward.

gziolo · 2018-05-02T09:55:02Z

core-data/index.js

+import entities from './entities';
+
+const entityResolvers = entities.reduce( ( memo, { kind, name } ) => {
+	const kindPrefix = kind === 'root' ? '' : upperFirst( camelCase( kind ) );


I would love to see the following method to avoid code duplication and ensure that both selector and resolver names always match:

const getMethodName = ( kind, name ) => { const kindPrefix = kind === 'root' ? '' : upperFirst( camelCase( kind ) ); const nameSuffix = upperFirst( camelCase( name ) ); return `get${ kindPrefix }${ nameSuffix }`; };

This logic is quite complicated and essential to have it working properly for all selectors so it would be great to have a simple unit test which ensures it never regresses.

Good suggestion, updating.

or:

const generateEntityMethods = ( source) => { const entityResolvers = entities.reduce( ( memo, { kind, name } ) => { const kindPrefix = kind === 'root' ? '' : upperFirst( camelCase( kind ) ); const nameSuffix = upperFirst( camelCase( name ) ); return { ...memo, [ `get${ kindPrefix }${ nameSuffix }` ]: ( state, key ) => source.getEntityRecord( state, kind, name, key ), }; }, {} ); const entityResolvesr = generateEntityMethods( resolvers ); const entitySelectors = generateEntityMethods( selectors );

gziolo

I'm giving 👍 to the implementation. I recommend waiting for another opinion before proceeding given the impact this PR is going to have. I personally love the direction and flexibility this PR introduces. In particular, this will make it very easy to have those entities extensible because of how those definitions are stored in one file: https://github.com/WordPress/gutenberg/blob/9609b27b885598deb495369f28bd19c11c7023a2/core-data/entities.js.

aduth · 2018-05-02T12:53:35Z

core-data/entities.js

+import { find, upperFirst, camelCase } from 'lodash';
+
+const entities = [
+	{ name: 'postType', kind: 'root', key: 'slug', baseUrl: '/wp/v2/types' },


Could kind just be optional rather than having an explicit 'root' value? Or does that make the implementation more complex to handle the fact it's optional?

In several methods, the kind is the first parameter before the name and other optional parameters (think query, for instance). I'd love to get rid of the kind but I fear these methods arguments won't be that easy to handle in that case. getEntityRecords( null, 'postType', query )

aduth · 2018-05-02T12:54:36Z

core-data/entities.js

+ * @return {Object} Entity
+ */
+export function getEntity( kind, name ) {
+	return find( entities, ( entity ) => entity.kind === kind && entity.name === name );


Nit: Object predicate could make this nice:

return find( entities, { kind, name } );

I though these shortcuts were getting deprecated from lodash?

As I understand it, they are on the roadmap to be excluded by default in 5.0.0 .

Remove shorthand support by default

https://github.com/lodash/lodash/wiki/Roadmap

Doesn't mean they won't exist as an option. I'm personally a fan, and would opt for it.

aduth · 2018-05-02T12:55:41Z

core-data/entities.js

+ * @return {string} Method name
+ */
+export const getMethodName = ( kind, name ) => {
+	const kindPrefix = kind === 'root' ? '' : upperFirst( camelCase( kind ) );


root optional might actually make this simpler.

aduth · 2018-05-02T12:59:58Z

core-data/index.js

@@ -10,12 +10,29 @@ import reducer from './reducer';
 import * as selectors from './selectors';
 import * as actions from './actions';
 import * as resolvers from './resolvers';
+import { default as entities, getMethodName } from './entities';
+
+const entityResolvers = entities.reduce( ( memo, { kind, name } ) => {


Personally I've stopped using the name memo because it seems to be common for it to be unfamiliar. Not sure if you feel strongly about it one way or the other.

What's your current way of doing this? do you use a specific name, like resolvers here for instance?

I tend toward result, though I'm not strongly committed to it.

aduth · 2018-05-02T13:00:27Z

core-data/index.js

+const entityResolvers = entities.reduce( ( memo, { kind, name } ) => {
+	const methodName = getMethodName( kind, name );
+	return {
+		...memo,


Minor (Perf): As implemented, we're creating a shallow clone on each iteration, when we could be mutating directly.

aduth · 2018-05-02T13:01:44Z

core-data/index.js

+	};
+}, {} );
+
+const entitySelectors = entities.reduce( ( memo, { kind, name } ) => {


My DRY senses are tingling 😄

Maybe:

const createEntityRecordGetter = ( source ) => entities.reduce( ( result, entity ) => { const { kind, name } = entity; const methodName = getMethodName( kind, name ); result[ methodName ] = ( state, key ) => source.getEntityRecord( state, kind, name, key ); return result; } );

I know me too, but at some point, I thought they could diverge (have more selectors than resolvers ). Of course, we can update later when they diverge.

I think I saw a similar comment earlier today 😄

Oof, the "Show outdated" toggles in GitHub are really easy to miss. 😬

Since you both asked for it, I executed :)

aduth · 2018-05-02T13:04:20Z

core-data/reducer.js


-	return state;
+		const key = entityConfig.key || 'id';


Minor (Perf): We don't need to derive this on every dispatch. It can be in the surrounding function scope.

youknowriad · 2018-05-02T14:50:00Z

Thanks all for your help on this PR

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 27, 2018

youknowriad self-assigned this Apr 27, 2018

youknowriad requested review from aduth and a team April 27, 2018 07:36

jorgefilipecosta reviewed Apr 28, 2018

View reviewed changes

gziolo reviewed Apr 30, 2018

View reviewed changes

aduth reviewed Apr 30, 2018

View reviewed changes

youknowriad changed the title ~~Initial models implementation~~ Initial entities implementation May 2, 2018

gziolo reviewed May 2, 2018

View reviewed changes

gziolo approved these changes May 2, 2018

View reviewed changes

aduth reviewed May 2, 2018

View reviewed changes

youknowriad added 7 commits May 2, 2018 15:35

Initial models implementation

977ecce

Addd unit tests

9112188

Use primaryKey instead of PK

1b7a1fb

Fix selectors unit tests

b2ab8fc

Move selectors/resolvers generators to the core-data/index.js

7c8b045

Rename Model to Entity

95b41b3

Rename primaryKey to just key

b395599

youknowriad added 5 commits May 2, 2018 15:37

Use the entity abstraction for media fetching

bc05167

Extract getMethodName helper

af5df29

Use lodash shortcut syntax

3129742

Factorize the resovers/selectors generation

e3a60bd

Optimize entity reducer

95ae266

youknowriad force-pushed the try/models branch from 6afc914 to 95ae266 Compare May 2, 2018 14:37

Fix small bug when generating selectors/resolvers

91662c1

youknowriad merged commit 239de2e into master May 2, 2018

youknowriad deleted the try/models branch May 2, 2018 14:50

youknowriad added this to the 2.8 milestone May 2, 2018

youknowriad mentioned this pull request May 16, 2018

Fix a race condition in withAPIData() #6303

Closed

aduth mentioned this pull request Jun 12, 2018

Framework: Add queried-data state helper utility #6395

Closed

ellatrix mentioned this pull request Dec 15, 2023

ToC: adapt to rich text value #57093

Closed

Initial entities implementation #6462

Initial entities implementation #6462

Conversation

youknowriad commented Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Apr 28, 2018 • edited Loading

youknowriad commented Apr 30, 2018

gziolo Apr 30, 2018 • 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

gziolo commented Apr 30, 2018

youknowriad commented Apr 30, 2018

youknowriad commented Apr 30, 2018

aduth commented Apr 30, 2018

aduth commented Apr 30, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth May 1, 2018 • 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

aduth May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 1, 2018

gziolo May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 2, 2018

youknowriad commented May 2, 2018

gziolo May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a 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

Choose a reason for hiding this comment

aduth May 2, 2018 • 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

Choose a reason for hiding this comment

aduth May 2, 2018 • 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

Choose a reason for hiding this comment

youknowriad commented May 2, 2018

youknowriad commented Apr 27, 2018 •

edited

Loading

jorgefilipecosta commented Apr 28, 2018 •

edited

Loading

gziolo Apr 30, 2018 •

edited

Loading

aduth May 1, 2018 •

edited

Loading

aduth May 1, 2018 •

edited

Loading

gziolo May 2, 2018 •

edited

Loading

gziolo May 2, 2018 •

edited

Loading

aduth May 2, 2018 •

edited

Loading

aduth May 2, 2018 •

edited

Loading