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

Framework: Use a single way to call APIs: wp.apiRequest #5253

Merged
merged 2 commits into from
Feb 28, 2018

Conversation

youknowriad
Copy link
Contributor

This PR refactors API calls in Gutenberg to always use wp.apiRequest.
In addition to consistency, this makes it easier for third-party Gutenberg USAGE to use it without a WordPress backend and just provide an alternative implementation to wp.apiRequest

Testing instructions

  • Test that saving/updating and removing posts works
  • Test that adding/removing tags and categories works
  • Test that embed blocks still work
  • Test that saving metaboxes work
  • Test that you can create/update and delete reusable blocks.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Feb 26, 2018
@youknowriad youknowriad self-assigned this Feb 26, 2018
setAttributes( { type, providerNameSlug } );
} else {
this.setState( { error: true } );
}
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the embed blocks here.

With fetch, we get the error response from oembed back here as json, so this is where we deal with errors. With wp.apiRequest, we don't. So if anyone puts in a URL that we can't embed, we never get out of the fetching state and report the error to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @notnownikki It should be fixed now.

Copy link
Member

Choose a reason for hiding this comment

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

👍 You beat me to the fix :) works as expected now!

@brandonpayton
Copy link
Member

This is beautiful. I had no idea it was considered reasonable to not use wp.api.models.

@gziolo
Copy link
Member

gziolo commented Feb 27, 2018

I like how you unified the way all API calls are handled. In particular, I love the fact that there is one way to trigger all calls. However, I'm not sold on the idea of leaving the details on how paths are created for the endpoints. This should be an implementation detail in my opinion. I would prefer having:

this.suggestionsRequest = wp.api.makeRequest( 'posts', {
	search: value,
	perPage: 20,
	orderBy: 'relevance',
} );

const makeRequest = ( endpointName, options ) => {
	const endpointMappers = applyFilters( 'api/endpointMappers',  {
		posts: ( { search, perPage, orderBy } ) = ( {
			path: `/wp/v2/posts?${ stringify( {
				search,
				per_page: perPage,
				orderby: orderBy,
			} ) }`,
		},
	} );
	// I assumed that the base path is prepended if not provided
	wp.apiRequest( endpointMappers[ endpointName ]( options ) );
};

over:

this.suggestionsRequest = wp.apiRequest( {
	path: `/wp/v2/posts?${ stringify( {
		search: value,
		per_page: 20,
		orderby: 'relevance',
	} ) }`,
} );

It would be definitely more work because you would have to define a separate mapping function for every single API endpoint. However, we could expose this configuration and make it extensible. This would allow overriding those mapping functions and give more control over how all those requests are handled.

I might be wrong in here because I'm not familiar with the code updated in this PR. So treat it as my own opinion. In fact, this PR is perfectly fine to be merged as it is once it is properly tested with someone who knows all the places we should look at.

@youknowriad
Copy link
Contributor Author

@gziolo Actually, I'm totally on board with your idea, in fact, I think that should be the next step. This is more "in-between" approach unifying API calls without rewriting everything.

In your proposal, we'll have to specify all the models properly (independently from the WP models):

  • Posts: What does a post mean for Gutenberg, what attributes it contains, what operations are allowed
  • Taxonomies: Same
  • Embeds: Same
  • Saved Blocks: Same

This means a huge impact on the code base of Gutenberg because we're abstracting the backend entirely, which is good because it means offline support, WP agnostic but requires way more work. The unifying using wp.apiRequest is a middleground solution without too much impact.

An adventurous implementation can still achieve the exact same thing by mapping the WP paths instead.

@notnownikki
Copy link
Member

I'd get behind that too. I'm trying to fix a situation where I need some abstraction between the API call and what the block does with it, so, happy to help out if we go this direction.

@gziolo
Copy link
Member

gziolo commented Feb 27, 2018

In fact, this PR is perfectly fine to be merged as it is once it is properly tested with someone who knows all the places we should look at.

Just wanted to repeat that I'm more than happy to see it merged as it is :D

@gziolo
Copy link
Member

gziolo commented Feb 27, 2018

An adventurous implementation can still achieve the exact same thing by mapping the WP paths instead.

I wouldn't describe it better in terms of overriding paths based on regular expressions. I agree it's totally doable 👍

@youknowriad youknowriad force-pushed the update/use-wp-api-request-everywhere branch from aaf075e to 07c449b Compare February 27, 2018 12:41
@youknowriad youknowriad merged commit 699d427 into master Feb 28, 2018
@youknowriad youknowriad deleted the update/use-wp-api-request-everywhere branch February 28, 2018 07:50
@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 28, 2018
@phpbits
Copy link
Contributor

phpbits commented Mar 3, 2018

@youknowriad can wp.apiRequest get custom post meta value and update it? I would like to do this onChange action for my select field. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants