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

Create withAPIData higher-order component for managing API data #1974

Merged
merged 5 commits into from Aug 22, 2017

Conversation

Projects
None yet
5 participants
@aduth
Member

aduth commented Jul 21, 2017

Related: #902

This pull request seeks to introduce a new withAPIData higher-order component to provide a simple interface for fetching, creating, updating, or deleting data from the REST API.

Out of the box, it includes:

  • Auto-fetching when your component mounts
  • Reusing cached data if request has already been made
  • Provides status updates so you can render accordingly
  • Trigger creation, updates, or deletes on data

Example:

function MyPost( { post } ) {
	if ( post.isLoading ) {
		return <div>Loading...</div>;
	}

	return <div>{ post.data.title.rendered }</div>;
}

export default withAPIData( ( props, { type } ) => ( {
	post: `/wp/v2/${ type( 'post' ) }/${ props.postId }`
} ) )( MyPost );

Implementation notes:

withApiData behaves similar to, and in some cases complements, React-Redux's connect function. It accepts props which can either be passed from the parent component, or composed from another higher-order component (e.g. connect), and returns an object of propName -> endpoint result.

Data-bound props take the shape of an object with a number of properties, depending on the methods supported for the particular endpoint:

  • GET
    • isLoading: Whether the resource is currently being fetched
    • data: The resource, available once fetch succeeds
    • get: Function to invoke a new fetch request
    • error: The error object, if the fetch failed
  • POST
    • isCreating: Whether the resource is currently being created
    • createdData: The created resource, available once create succeeds
    • create: Function to invoke a new create request
    • createError: The error object, if the create failed
  • PUT
    • isSaving: Whether the resource is currently being saved
    • savedData: The saved resource, available once save succeeds
    • save: Function to invoke a new save request
    • saveError: The error object, if the save failed
  • PATCH
    • isPatching: Whether the resource is currently being patched
    • patchedData: The patched resource, available once patch succeeds
    • patch: Function to invoke a new patch request
    • patchError: The error object, if the patch failed
  • DELETE
    • isDeleting: Whether the resource is currently being deleted
    • deletedData: The deleted resource, available once delete succeeds
    • delete: Function to invoke a new delete request
    • deleteError: The error object, if the delete failed

There is some inspiration here from Heroku's react-refetch project.

Testing instructions:

Verify that there are no regressions in the LastRevision component (requires post with at least one revision to appear).


See original pull request comment: https://gist.github.com/aduth/e5dd6327963475e9f580b8ece523ffa0

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jul 27, 2017

Contributor

Thanks for exploring this. It will be important to streamline communication with the API within blocks for people to build from. I like that it is both explicit and encapsulates concerns fairly well. Compared to usual data-wrappers it also allows more flexibility in what data you are interested in (say, multiple endpoints). How do you see this being used in a block?

Contributor

mtias commented Jul 27, 2017

Thanks for exploring this. It will be important to streamline communication with the API within blocks for people to build from. I like that it is both explicit and encapsulates concerns fairly well. Compared to usual data-wrappers it also allows more flexibility in what data you are interested in (say, multiple endpoints). How do you see this being used in a block?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 27, 2017

Member

It works well for the block use-case because it's not dependent on a specific Redux store context existing. It can wrap any component; including edit and save, whether they're proper component classes, or even the simple function form:

edit: withApiData( ( props, endpoint ) => {
     // ..
} )( ( props ) => {
    // ...
} )) 
Member

aduth commented Jul 27, 2017

It works well for the block use-case because it's not dependent on a specific Redux store context existing. It can wrap any component; including edit and save, whether they're proper component classes, or even the simple function form:

edit: withApiData( ( props, endpoint ) => {
     // ..
} )( ( props ) => {
    // ...
} )) 
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 2, 2017

Member

I may have been a little overzealous with the tagged template literal. It might be possible to recreate the same behavior with a simple array:

export default withApiData( ( props ) => ( {
	revisions: [ '/wp/v2/posts', props.postId, 'revisions' ]
} ) )( MyPost );

Or sprintf style:

export default withApiData( ( props ) => ( {
	revisions: [ '/wp/v2/posts/%d/revisions', props.postId ]
} ) )( MyPost );
Member

aduth commented Aug 2, 2017

I may have been a little overzealous with the tagged template literal. It might be possible to recreate the same behavior with a simple array:

export default withApiData( ( props ) => ( {
	revisions: [ '/wp/v2/posts', props.postId, 'revisions' ]
} ) )( MyPost );

Or sprintf style:

export default withApiData( ( props ) => ( {
	revisions: [ '/wp/v2/posts/%d/revisions', props.postId ]
} ) )( MyPost );
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 3, 2017

Contributor

We need something like that. I suspect the WP async data needs will overpass the REST API at some point, maybe we'd need something more generic (QueryComponents, or something else), but I'm fine with this approach. We need something to move forward with several issues.

Contributor

youknowriad commented Aug 3, 2017

We need something like that. I suspect the WP async data needs will overpass the REST API at some point, maybe we'd need something more generic (QueryComponents, or something else), but I'm fine with this approach. We need something to move forward with several issues.

@sirbrillig

This comment has been minimized.

Show comment
Hide comment
@sirbrillig

sirbrillig Aug 3, 2017

Interesting idea! I like it. It has some similarities to the mapApiToProps() HOC I made for one of the precursors to Gutenberg.

For the editor view, we use a React Higher Order Component function called apiDataWrapper(). The function accepts a mapping function to map the api data into props for the component. The api data can be accessed using the special function getApiEndpoint() which is passed as the first argument to the mapping function. If the data is not available yet, it will be fetched.

sirbrillig commented Aug 3, 2017

Interesting idea! I like it. It has some similarities to the mapApiToProps() HOC I made for one of the precursors to Gutenberg.

For the editor view, we use a React Higher Order Component function called apiDataWrapper(). The function accepts a mapping function to map the api data into props for the component. The api data can be accessed using the special function getApiEndpoint() which is passed as the first argument to the mapping function. If the data is not available yet, it will be fetched.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 19, 2017

Member

I took to breathing life back into this pull request, rebasing to resolve conflicts and moving in a direction away from tagged template literals toward... just simple strings.

// Before:
export default withAPIData( ( props, endpoint ) => ( {
	post: endpoint`/wp/v2/posts/${ props.postId }`
} ) )( MyPost );

// After:
export default withAPIData( ( props, { type } ) => ( {
	post: `/wp/v2/${ type( 'post' ) }/${ props.postId }`
} ) )( MyPost );

Included also is:

  • Request caching
  • All request methods
  • Post type and taxonomy rest_base mapping

Fortunately I was able to make use of the newly extracted wp.apiRequest to avoid managing nonce and path concatenation myself (see: https://core.trac.wordpress.org/ticket/40919). Now that I'm considering it, I think the one remaining task is figuring out how to polyfill this method for users not running the 4.9 alpha (it doesn't exist in 4.8).

Member

aduth commented Aug 19, 2017

I took to breathing life back into this pull request, rebasing to resolve conflicts and moving in a direction away from tagged template literals toward... just simple strings.

// Before:
export default withAPIData( ( props, endpoint ) => ( {
	post: endpoint`/wp/v2/posts/${ props.postId }`
} ) )( MyPost );

// After:
export default withAPIData( ( props, { type } ) => ( {
	post: `/wp/v2/${ type( 'post' ) }/${ props.postId }`
} ) )( MyPost );

Included also is:

  • Request caching
  • All request methods
  • Post type and taxonomy rest_base mapping

Fortunately I was able to make use of the newly extracted wp.apiRequest to avoid managing nonce and path concatenation myself (see: https://core.trac.wordpress.org/ticket/40919). Now that I'm considering it, I think the one remaining task is figuring out how to polyfill this method for users not running the 4.9 alpha (it doesn't exist in 4.8).

@aduth aduth changed the title from Try: Create withApiData higher-order component for managing API data to Create withApiData higher-order component for managing API data Aug 19, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 21, 2017

Member

This is about ready to be merged, but it has me thinking: how do we handle invalidating data? For example, in the included port of LastRevision: Shouldn't the revisions refresh when the post is saved? This is of course a very difficult problem to solve, but I wonder if we might define dependencies between endpoints.

We might be able to infer some of this from the server schema (endpoint arguments):

  • Arguments might not be specific enough (id could be a post, user, etc)
  • _links metadata is a little more promising: from a single post entity, we know its collection, author, replies, version-history, etc, so we could invalidate based on this information.
    • I expect there could be some gaps here, particularly with plugin-authored endpoints.
    • This could be more destructive than necessary, i.e. it's unlikely we need to clear user cache when a post is saved

Defining these dependencies in the client could give us some patterns to create optimistic updates (i.e. "what should happen to data if we assume success"). Some prior art includes:

Another problem is data freshness, although this isn't as much of an issue until the admin becomes more like a single-page application. Solutions could include time-to-live (TTL) freshness, or lifecycle-based refresh (see Calypso query components). If data dependencies/mutation effects are well defined in the client, this may not be necessary at all.

In cases where a component must have fresh data, I think we could extend the mapping value to be an object of settings, optional as an alternative to the string definition:

export default withAPIData( ( props, { type } ) => ( {
	post: {
		path: `/wp/v2/${ type( 'post' ) }/${ props.postId }`,
		force: true,
	},
} ) )( MyPost );

tl;dr: two hard things in computer science

Member

aduth commented Aug 21, 2017

This is about ready to be merged, but it has me thinking: how do we handle invalidating data? For example, in the included port of LastRevision: Shouldn't the revisions refresh when the post is saved? This is of course a very difficult problem to solve, but I wonder if we might define dependencies between endpoints.

We might be able to infer some of this from the server schema (endpoint arguments):

  • Arguments might not be specific enough (id could be a post, user, etc)
  • _links metadata is a little more promising: from a single post entity, we know its collection, author, replies, version-history, etc, so we could invalidate based on this information.
    • I expect there could be some gaps here, particularly with plugin-authored endpoints.
    • This could be more destructive than necessary, i.e. it's unlikely we need to clear user cache when a post is saved

Defining these dependencies in the client could give us some patterns to create optimistic updates (i.e. "what should happen to data if we assume success"). Some prior art includes:

Another problem is data freshness, although this isn't as much of an issue until the admin becomes more like a single-page application. Solutions could include time-to-live (TTL) freshness, or lifecycle-based refresh (see Calypso query components). If data dependencies/mutation effects are well defined in the client, this may not be necessary at all.

In cases where a component must have fresh data, I think we could extend the mapping value to be an object of settings, optional as an alternative to the string definition:

export default withAPIData( ( props, { type } ) => ( {
	post: {
		path: `/wp/v2/${ type( 'post' ) }/${ props.postId }`,
		force: true,
	},
} ) )( MyPost );

tl;dr: two hard things in computer science

@aduth aduth changed the title from Create withApiData higher-order component for managing API data to Create withAPIData higher-order component for managing API data Aug 22, 2017

aduth added some commits Jul 21, 2017

Scripts: Move shim registration into client-assets
The plugin bundler will not pick up on scripts registered outside this file, and it is simpler to add here than enhance bundler to force reigstration of the shim from compat
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 22, 2017

Codecov Report

Merging #1974 into master will increase coverage by 1.37%.
The diff coverage is 77.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1974      +/-   ##
==========================================
+ Coverage   27.28%   28.65%   +1.37%     
==========================================
  Files         161      165       +4     
  Lines        4955     5039      +84     
  Branches      826      830       +4     
==========================================
+ Hits         1352     1444      +92     
- Misses       3051     3052       +1     
+ Partials      552      543       -9
Impacted Files Coverage Δ
editor/index.js 0% <ø> (ø) ⬆️
editor/sidebar/last-revision/index.js 0% <0%> (ø) ⬆️
components/higher-order/with-api-data/routes.js 100% <100%> (ø)
components/higher-order/with-api-data/request.js 100% <100%> (ø)
components/higher-order/with-api-data/provider.js 25% <25%> (ø)
components/higher-order/with-api-data/index.js 82.66% <82.66%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c65a572...0b98816. Read the comment docs.

codecov bot commented Aug 22, 2017

Codecov Report

Merging #1974 into master will increase coverage by 1.37%.
The diff coverage is 77.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1974      +/-   ##
==========================================
+ Coverage   27.28%   28.65%   +1.37%     
==========================================
  Files         161      165       +4     
  Lines        4955     5039      +84     
  Branches      826      830       +4     
==========================================
+ Hits         1352     1444      +92     
- Misses       3051     3052       +1     
+ Partials      552      543       -9
Impacted Files Coverage Δ
editor/index.js 0% <ø> (ø) ⬆️
editor/sidebar/last-revision/index.js 0% <0%> (ø) ⬆️
components/higher-order/with-api-data/routes.js 100% <100%> (ø)
components/higher-order/with-api-data/request.js 100% <100%> (ø)
components/higher-order/with-api-data/provider.js 25% <25%> (ø)
components/higher-order/with-api-data/index.js 82.66% <82.66%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c65a572...0b98816. Read the comment docs.

@aduth aduth merged commit 67e759b into master Aug 22, 2017

3 checks passed

codecov/project 28.65% (+1.37%) compared to c65a572
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the add/api-hoc branch Aug 22, 2017

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2017

Contributor

Several endpoints support pagination, any ideas on how to support pagination in this HoC?

Contributor

youknowriad commented Aug 23, 2017

Several endpoints support pagination, any ideas on how to support pagination in this HoC?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 23, 2017

Member

I touched on pagination in the "Implementation notes" of #2501:

While not entirely necessary for the changes here, I split the response between body and headers. This will be useful for future enhancements to the withAPIData higher-order component in allowing a "has more results?" type functionality.

Specifically, paginated endpoints will return response headers which can be used to determine whether there are more results available (X-WP-Total, X-WP-TotalPages).

Maybe these are exposed as properties on the mapped prop, e.g. this.props.posts.hasMore or this.props.posts.totalPages.

More generally, we might want to track total number of pages regardless of the query parameters of the original request which had initiated it (example).

We could get more sophisticated here too using PaginatedQueryManager from Calypso which emulates the effects of pagination.

Member

aduth commented Aug 23, 2017

I touched on pagination in the "Implementation notes" of #2501:

While not entirely necessary for the changes here, I split the response between body and headers. This will be useful for future enhancements to the withAPIData higher-order component in allowing a "has more results?" type functionality.

Specifically, paginated endpoints will return response headers which can be used to determine whether there are more results available (X-WP-Total, X-WP-TotalPages).

Maybe these are exposed as properties on the mapped prop, e.g. this.props.posts.hasMore or this.props.posts.totalPages.

More generally, we might want to track total number of pages regardless of the query parameters of the original request which had initiated it (example).

We could get more sophisticated here too using PaginatedQueryManager from Calypso which emulates the effects of pagination.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 23, 2017

Member

Example:

class PagedPosts extends Component {
	state = {
		page: 1
	};

	onNextPage = ( increment ) => {
		const { page } = this.state;
		this.setState( { page: page + increment } );
	};

	render() {
		return (
			<PagedPostsPage 
				page={ this.state.page }
				onNextPage={ this.onNextPage } />
		);
	}
}

const PagedPostsPage = withAPIData( ( { page } ) => ( {
	posts: `/wp/v2/posts?page=${ page }`
} ) )( ( { posts, onNextPage } ) => (
	<div>
		{ posts.data && (
			<ul>
				{ posts.data.map( ( post ) => (
					<li>{ post.title.rendered }</li>
				) ) }
			</ul>
		) }
		{ page !== 1 && (
			<Button onClick={ () => onNextPage( -1 ) }>
				Previous
			</Button>
		) }
		{ posts.hasMore && (
			<Button onClick={ () => onNextPage( 1 ) }>
				Next
			</Button>
		) }
	</div>
) );
Member

aduth commented Aug 23, 2017

Example:

class PagedPosts extends Component {
	state = {
		page: 1
	};

	onNextPage = ( increment ) => {
		const { page } = this.state;
		this.setState( { page: page + increment } );
	};

	render() {
		return (
			<PagedPostsPage 
				page={ this.state.page }
				onNextPage={ this.onNextPage } />
		);
	}
}

const PagedPostsPage = withAPIData( ( { page } ) => ( {
	posts: `/wp/v2/posts?page=${ page }`
} ) )( ( { posts, onNextPage } ) => (
	<div>
		{ posts.data && (
			<ul>
				{ posts.data.map( ( post ) => (
					<li>{ post.title.rendered }</li>
				) ) }
			</ul>
		) }
		{ page !== 1 && (
			<Button onClick={ () => onNextPage( -1 ) }>
				Previous
			</Button>
		) }
		{ posts.hasMore && (
			<Button onClick={ () => onNextPage( 1 ) }>
				Next
			</Button>
		) }
	</div>
) );
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 23, 2017

Contributor

Thanks, @aduth really helpful.

Do you think we should add support of multiple pages (multiple requests with the same HoC) with the same wothAPIData wrapper? I'm thinking this could be more flexible, you can do whatever you want with the result. Maybe It's too premature though and not needed.

Contributor

youknowriad commented Aug 23, 2017

Thanks, @aduth really helpful.

Do you think we should add support of multiple pages (multiple requests with the same HoC) with the same wothAPIData wrapper? I'm thinking this could be more flexible, you can do whatever you want with the result. Maybe It's too premature though and not needed.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 23, 2017

Member

You could currently do something like:

withAPIData( {
	page1: `/wp/v2/posts?page=1`,
	page2: `/wp/v2/posts?page=2`,
} )( MyList );

But I think in general cases it might be better to have a parent component which manages those individual pages, as in the above example.

Let's revisit if we encounter it being an issue in practice.

Member

aduth commented Aug 23, 2017

You could currently do something like:

withAPIData( {
	page1: `/wp/v2/posts?page=1`,
	page2: `/wp/v2/posts?page=2`,
} )( MyList );

But I think in general cases it might be better to have a parent component which manages those individual pages, as in the above example.

Let's revisit if we encounter it being an issue in practice.

// See: gutenberg_ensure_wp_api_request (compat.php).
gutenberg_register_vendor_script(
'wp-api-request-shim',
'https://rawgit.com/WordPress/wordpress-develop/master/src/wp-includes/js/api-request.js'

This comment has been minimized.

@mcsf

mcsf Sep 6, 2017

Contributor

@aduth, even though this is a temporary measure, shouldn't we prefer a cdn.rawgit.com-type URL?

Per their homepage:

[cdn.rawgit.com] No traffic limits or throttling. Files are served via StackPath's super fast global CDN.

vs.

[rawgit.com] Excessive traffic will be throttled and blacklisted.

@mcsf

mcsf Sep 6, 2017

Contributor

@aduth, even though this is a temporary measure, shouldn't we prefer a cdn.rawgit.com-type URL?

Per their homepage:

[cdn.rawgit.com] No traffic limits or throttling. Files are served via StackPath's super fast global CDN.

vs.

[rawgit.com] Excessive traffic will be throttled and blacklisted.

This comment has been minimized.

@aduth

aduth Sep 7, 2017

Member

Now that you mention it, I'd meant to look if we need to use rawgit.com at all, or if we can just reference the GitHub.com link directly. The latter is blocked for script tags in the browser, but since this occurs server-side, we might be able to reference the raw JS file straight from the source.

@aduth

aduth Sep 7, 2017

Member

Now that you mention it, I'd meant to look if we need to use rawgit.com at all, or if we can just reference the GitHub.com link directly. The latter is blocked for script tags in the browser, but since this occurs server-side, we might be able to reference the raw JS file straight from the source.

This comment has been minimized.

@aduth

aduth Sep 7, 2017

Member

Also worth noting that we're not sending much traffic here. The plugin includes a pre-downloaded copy, and in development the script is downloaded and cached for 24 hours.

@aduth

aduth Sep 7, 2017

Member

Also worth noting that we're not sending much traffic here. The plugin includes a pre-downloaded copy, and in development the script is downloaded and cached for 24 hours.

This comment has been minimized.

@mcsf

mcsf Sep 8, 2017

Contributor

Fair enough. :)

@mcsf

mcsf Sep 8, 2017

Contributor

Fair enough. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment