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

Data Module: Add the getEntityRecords support #6939

Merged
merged 2 commits into from May 25, 2018

Conversation

youknowriad
Copy link
Contributor

closes #6780

This PR adds support to the getEntityRecords selectors/resolvers to the core data module to avoid duplication across the different entities.

Testing instructions

  • Ensures that the post taxonomies panel show up properly

@youknowriad youknowriad added the [Package] Data /packages/data label May 24, 2018
@youknowriad youknowriad self-assigned this May 24, 2018
const received = ( await fulfillment.next() ).value;
expect( received ).toEqual( receiveEntityRecords( 'root', 'postType', Object.values( POST_TYPES ) ) );
} );
} );
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worthwhile to have taxonomy tests here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. I think it's worthwhile is to add tests to ensure the selectors/resolvers are generated (the code in the core-data/index.js) but several parts of this code are already tests: the methodName and the underlying selectors/resolvers.

Copy link
Member

Choose a reason for hiding this comment

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

👍

export async function* getTaxonomies() {
const taxonomies = await apiRequest( { path: '/wp/v2/taxonomies?context=edit' } );
yield receiveTaxonomies( taxonomies );
export async function* getEntityRecords( state, kind, name ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I'm missing JSDoc here

@youknowriad youknowriad merged commit 37c54af into master May 25, 2018
@youknowriad youknowriad deleted the add/get-entities-support branch May 25, 2018 12:29
},
( state, kind, name ) => ( [
state.entities[ kind ][ name ].byKey,
] )
Copy link
Member

Choose a reason for hiding this comment

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

Tab should be space before the closing parenthesis.

Copy link
Member

Choose a reason for hiding this comment

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

Or just don't use redundant parentheses. Unless the argument is they add clarity to disambiguate between [ and { (but then, what about implicit returns?).

@@ -5,7 +5,8 @@ import { find, upperFirst, camelCase } from 'lodash';

const entities = [
{ name: 'postType', kind: 'root', key: 'slug', baseUrl: '/wp/v2/types' },
{ name: 'media', kind: 'root', baseUrl: '/wp/v2/media' },
{ name: 'media', plural: 'mediaItems', kind: 'root', baseUrl: '/wp/v2/media' },
{ name: 'taxonomy', kind: 'root', key: 'slug', baseUrl: '/wp/v2/taxonomies', plural: 'taxonomies' },
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Inconsistent property ordering.

  • Previous line: name, plural, kind, baseUrl
  • This line: name, kind, key, baseUrl, plural

const nameSuffix = upperFirst( camelCase( name ) );
return `get${ kindPrefix }${ nameSuffix }`;
const nameSuffix = upperFirst( camelCase( name ) ) + ( usePlural ? 's' : '' );
const suffix = usePlural && entity.plural ? upperFirst( camelCase( entity.plural ) ) : nameSuffix;
Copy link
Member

Choose a reason for hiding this comment

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

The previous line is wasted work if usePlural && entity.plural is truthy. Further, should this be extracted to a utility? Or refactored to logic where we consider a custom plural value before doing the 's' concatenation.

Aside: On the fence about whether taking on this responsibility from the developer via some utility like pluralize could be a good thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know but I wanted to avoid the embedded ifs or ternaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage getEntityRecord when getting taxonomies from core-data
3 participants