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

Move Post Types Data Fetching to the core-data module #5744

Merged
merged 2 commits into from
Mar 29, 2018

Conversation

youknowriad
Copy link
Contributor

Follow-up to #5219

This PR refactors the usage of withAPIData to fetch the current post types to the data module.

  • It adds a sub-state tree to manager post types
  • It refactors all the components relying on this data to use withSelect and withDispatch instead of connect and withAPIData.

@youknowriad youknowriad self-assigned this Mar 22, 2018
@youknowriad youknowriad requested a review from aduth March 22, 2018 10:35
@youknowriad youknowriad force-pushed the add/post-types-core-data branch from 887d6b4 to abb96ef Compare March 22, 2018 10:39
* @param {number} slug Post Type slug
*/
export async function* getPostType( state, slug ) {
const postType = await apiRequest( { path: `/wp/v2/types/${ slug }?context=edit` } );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context is hard-coded for now. As it's the only context we use right now.
I wonder how should we approach this:

  • Always use edit assuming this context returns the full object?
  • Store received data per context and make the context and argument of the resolver (I don't like this)
  • Something else (maybe server-side)?

What's your take on this?

Copy link
Member

Choose a reason for hiding this comment

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

It's a quite interesting question. It looks like view is the default one: https://developer.wordpress.org/rest-api/reference/post-types/#arguments. Given that this can be also customized by the user we should come up with a general solution which allows to provide this value. I have a more general question, why we have edit here and view in other places? Shouldn't we use the same context for all requests on a given page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use edit mainly because it contains more information. For me it's, this argument is a bit ambiguous because it assumes that it has to do with rights but rights shouldn't be asked for using request arguments. The server could just reply with all the fields the user can see (unless we ask for specific fields using the fields arg)

Copy link
Member

Choose a reason for hiding this comment

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

My naive assumption is that context should be defined on the server when loading a given page and automatically attached to all requests. This would enforce consistency and provide a unified way of handling all endpoints.

Copy link
Member

Choose a reason for hiding this comment

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

Tough question. Another option is to make selectors more granular, so that we know based on the selector being called whether we need to provide context=edit (i.e. Why do we need it specifically for what we're refactoring? Can those needs be expressed as distinct selectors?). Of course, this means we run into other issues of how to avoid duplicate requests amongst all of the selectors' resolvers.

A main annoyance for me with this context business is that it's authorized as though the user intends to edit specific values, but in many cases it's just generally useful information (like labels of a post type) which doesn't seem to be such sensitive information, at least to be blocking users of certain roles from accessing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! let's move forward with the current implementation (forcing edit) and see how it plays out with other data.

@youknowriad youknowriad requested review from gziolo and a team March 27, 2018 09:02
@youknowriad
Copy link
Contributor Author

I'd appreciate a review on this one :)

@youknowriad youknowriad force-pushed the add/post-types-core-data branch from abb96ef to c852837 Compare March 28, 2018 11:49
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I left my comments. I will test this PR once my feedback is addressed.

It is almost ready to go, I checked again my comments and those are minor things. I really like how it all looks with resolvers. Great API 💯

* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {string} Updated state.
Copy link
Member

Choose a reason for hiding this comment

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

Should return Object :)

case 'RECEIVE_POST_TYPES':
return {
...state,
...keyBy( action.postTypes, 'slug' ),
Copy link
Member

Choose a reason for hiding this comment

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

Love this lodash library, they should ship it with every browser by default!!! :trollface:

* @param {number} slug Post Type slug
*/
export async function* getPostType( state, slug ) {
const postType = await apiRequest( { path: `/wp/v2/types/${ slug }?context=edit` } );
Copy link
Member

Choose a reason for hiding this comment

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

It's a quite interesting question. It looks like view is the default one: https://developer.wordpress.org/rest-api/reference/post-types/#arguments. Given that this can be also customized by the user we should come up with a general solution which allows to provide this value. I have a more general question, why we have edit here and view in other places? Shouldn't we use the same context for all requests on a given page?

@@ -30,7 +23,7 @@ function FeaturedImage( { isOpened, postType, onTogglePanel } ) {
<PanelBody
title={ get(
postType,
[ 'data', 'labels', 'featured_image' ],
[ 'labels', 'featured_image' ],
Copy link
Member

Choose a reason for hiding this comment

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

Can we also camelCaseDeepKeys all keys in the future? :)

I have it implemented in coediting PR: https://github.com/WordPress/gutenberg/blob/35df0a590c0230519ceb36616d2859ed1d0c8888/coediting/utils/index.js

We could apply camelCaseDeepKeys to all responses as part of the resolver processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we go this direction, I don't think we should apply camelCaseDeepKeys arbitrarily but more create well defined models for all our entities (we don't have to match the API object at all). I think this is a large subject we should probably discuss separately and come up with a plan.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, definitely not for this PR. I need to be more cautious when leaving random thoughts and state it clearly that I just want to open an unrelated discussion :)

We can defer this one, but it would be nice to have well-defined models validated with JSON schema. Ideally automated 😃

const { getEditedPostAttribute } = select( 'core/editor' );
const { getPostType } = select( 'core' );
return {
postType: getPostType( getEditedPostAttribute( 'type' ) ),
Copy link
Member

Choose a reason for hiding this comment

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

General observation. It looks like getCurrentPostType should be deprecated and replaced with getEditedPostAttribute in all other places with the follow-up PRs.

applyWithAPIDataItems,
withInstanceId,
] )( PageAttributesParent );
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why withInstanceId was removed? What is the current status of this HOC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was not used in the component, I think it was a left over.

return {
postId: getCurrentPostId(),
parent: getEditedPostAttribute( 'parent' ),
postType: getPostType( postTypeSlug ),
Copy link
Member

Choose a reason for hiding this comment

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

postType contains slug, so there is no need to pass it as a separate prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But postType is not necessarily loaded, that was my thinking.

Copy link
Member

Choose a reason for hiding this comment

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

postTypeSlug - the same applies. We can leave it as it is :)

return {
postType: `/wp/v2/types/${ postTypeSlug }?context=edit`,
onUpdateParent( parent ) {
editPost( { parent: parent || 0 } );
Copy link
Member

Choose a reason for hiding this comment

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

return is missing. Not sure it is ever used but noting the difference with the original code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, i think it's useless. We needed the return previously because it was not auto-bound to dispatch.

return {
postType: postTypeName ? `/wp/v2/types/${ postTypeName }?context=edit` : undefined,
media: featuredImageId ? getMedia( featuredImageId ) : null,
postType: getPostType( select( 'core/editor' ).getEditedPostAttribute( 'type' ) ),
Copy link
Member

Choose a reason for hiding this comment

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

Can be simplified to:

postType: getPostType( getEditedPostAttribute( 'type' ) ),

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

My test posts work as before. I don't see any regressions or errors on the JS console.

@youknowriad youknowriad merged commit 112c752 into master Mar 29, 2018
@youknowriad youknowriad deleted the add/post-types-core-data branch March 29, 2018 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants