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.

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

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

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

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 :)

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.

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 (#) ?

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.

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

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.

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.

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 );

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 )

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.

root optional might actually make this simpler.

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.

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

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 :)

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