Try a modular "required data" approach for fresh API data. #22961

coderkevin · 2018-03-02T21:56:43Z

Note: I am looking for feedback on this PR, for the ease of use, the structure of the code, and even the names of things. This is certainly not the final state of the code, but it is functional and complete enough to share with others and talk about.

This is the culmination of several iterations on a general-purpose data
requirement approach which endeavors to provide data to React components
in the simplest, most declarative way possible. It also endeavors to be
easy to hook in new APIs, endpoints, and data selectors. For the
endpoints, selectors, and React Components, it becomes possible to use
them on multiple platforms by implementing only the API layer below it in each
environment. (e.g. using data-layer on Calypso and withApiData, etc.
on wp-admin.)

Goals:

Handle API data in the easiest, most declarative way possible within the Application.
Make something that could work on both Calypso and wp-admin
Make it very easy to add new API endpoints and selectors.

To Test:

Open your JS dev console and enter in localStorage.setItem( 'debug', 'rest-api-client:*' ); Leave it open to watch logging.
Go to a product edit page.
Open that same product in another tab in wp-admin.
In wp-admin, edit the product's name and save it.
Observe the Store product edit page and logging to watch the name update automatically within 30 seconds.

Major pieces of the code:

registry: Registers apiTypes (e.g. 'woocommerce'). Holds a module global list of api-client objects that can be looked up via getApiClient.
api-client: Handles a single type of api for a given site, identified by a siteKey (for Calypso this is a numeric site id, but for wp-admin this will likely be the base url of the site (e.g. mysubdomain.example.com or example.com/mymultisite2)
ApiClientProvider: Maps the root API State to Redux, and triggers requirement updates whenever it changes.
WithApiClient: This is a HOC that provides selectors that both retrieve data from state and establish a component's data requirements. This is what the application developer sees.
apiType: An object that defines endpoints and selectors. This is what the API developer sees.
methods: Provided to apiTypes on creation. These are platform-specific (e.g. Calypso or wp-admin).

This code exhibits the full-circle path of fetching a product for the product edit page. This is what it does:

Set API data requirement in the product-form component: apiClient.selectors.getProduct( productId, { freshness: 30 * SECOND } );
The with-api-client maps the selector data to props (using mapStateToProps), and also updates the requirements information for that particular data within the corresponding ApiClient code. The act of updating requirements triggers an update cycle within the ApiClient.
The update cycle checks for any updates that need to occur, dispatches actions for them, and determines the next time an update will be needed based on freshness requirements. It schedules an update using setTimeout.
When the product API response arrives, it sets the appropriate data in redux state via a reducer.
The ApiClientProvider is connected to all API State, and fires off an update cycle to re-evaluate data requirements and reset the timer.
product-form is redux connect-ed through with-api-client and also responds to the change in state for the new API data. It re-renders and reflects the new data.

Things that can trigger a requirements update cycle:

Changing requirement(s)
Receiving API data
Timer activation, due to freshness expiration.

This is the culmination of several iterations on a general-purpose data requirement approach which endeavors to provide data to React components in the simplest, most declarative way possible. It also endeavors to be easy to hook in new APIs, endpoints, and data selectors. For the endpoints, selectors, and React Components, it becomes possible to use them cross-platform by implementing only the API layer below it in each environment. (e.g. using `data-layer` on Calypso and `withApiData`, etc. on wp-admin.

matticbot · 2018-03-02T21:56:44Z

Test live: https://calypso.live/?branch=try/rest-api-require-data

This improves the testability of the registry by making all module globals optional argument defaults.

timmyc

Very cool stuff Kevin. I was able to follow along mostly with the code, but am hopeful some authors of similar modules ( like @dmsnell ) can give this a more thorough review, because it is definitely over my head in places 😆

I do think this would be a great PR to do a demo of during a team hangout if you are up for it.

I did perform the test, and here are a few take-aways for me:

The freshness feature is quite powerful, and it can solve some open issues we have in Store around the order list not refreshing. But it does appear to keep polling perpetually. For example, I opened up a product edit screen, then went and read some p2s, and saw this upon return in the network tab:

Freshness is awesome, and makes sense in many cases, but I fear if we have a page where we query multiple endpoints ( thinking the dashboard for example ) this could get pretty chatty. I suppose we could limit freshness to a few key endpoints, but it is also something I could see unsuspecting developers ( cough me ) using incorrectly and creating a bunch of API traffic. With our history of issues with the Jetpack Proxy endpoint and timeouts, this is a concern for me.

In my test, I added a product image, and updated the description. It was magical to see the image appear on the edit page, and subsequently in the product listing page. The product description did not refresh on the edit page. Guessing there is some tinymce funkyness going on there.

I will continue to play with it some more, but wanted to chime in with my findings thus far.

timmyc · 2018-03-09T00:22:34Z

client/extensions/woocommerce/app/products/product-form.js

+
+function mapApiToProps( apiClient, ownProps, state ) {
+	const { siteId, productId } = ownProps;
+	const apiProduct = apiClient.selectors.getProduct( productId, { freshness: 30 * SECOND } );


Just so I'm following along correctly, this is saying, get a product from the API, if the local copy in the state tree is older than 30 seconds?

I read the FULL pr description more carefully and answered my question.

Freshness is awesome, and makes sense in many cases, but I fear if we have a page where we query multiple endpoints ( thinking the dashboard for example ) this could get pretty chatty.

Definitely. In future PRs, I plan to add a feature that will stop fetching all data when the user is inactive. I'm also open to other suggestions.

it is also something I could see unsuspecting developers ( cough me ) using incorrectly and creating a bunch of API traffic.

I'm not sure how to police our own developers on this, or if we should. There are plenty of use cases where fairly frequent polling might make sense. Maybe a minimum freshness that can be enforced as a part of the API settings? (in state/wc-api for this case) Or maybe we could set a minimum freshness setting on each endpoint, so if there's an endpoint that should be polled more or less often, we could set that.

The product description did not refresh on the edit page. Guessing there is some tinymce funkyness going on there.

I saw the same behavior. I think our TinyMCE editor component is probably dropping the ball here.

coderkevin · 2018-03-13T00:36:02Z

Thanks for the initial review, Timmy! I'd be happy to demo this.

ryelle

I'm not totally clear on what should be a 3rd-party "generic" handler, vs what is WC specific, vs what is needed in Calypso - is there still the idea that part of this would be an external module?

ryelle · 2018-03-13T16:48:15Z

client/extensions/woocommerce/app/index.js

-				<QueryJetpackPlugins siteIds={ [ siteId ] } />
-				{ children }
-			</div>
+			<ApiClientProvider apiReduxRoot={ apiReduxRoot }>


Maybe this prop could just be root?

I don't have a problem with it, but do you think it would be descriptive enough?

ryelle · 2018-03-13T16:55:01Z

client/extensions/woocommerce/app/products/product-form.js

+
+function mapApiToProps( apiClient, ownProps, state ) {
+	const { siteId, productId } = ownProps;
+	const apiProduct = apiClient.selectors.getProduct( productId, { freshness: 30 * SECOND } );


I see from below why this is .selectors.getProduct, but that word has a meaning now (of selecting data out of an existing state) – maybe this could be .data.getProduct?

And state here is the global state, correct?

I'm definitely up for changes to naming for this. In fact, if there's nothing else that we need from apiClient other than the selectors, maybe we can just pass that in instead.

ryelle · 2018-03-13T17:13:22Z

client/extensions/woocommerce/app/products/product-form.js

+	};
+}
+
+export default withApiClient( 'woocommerce', mapApiToProps, 'siteId' )( ProductForm );


The arguments here are confusing, what do you think about converting it to an object, so they can be named?

{ type: 'woocommerce', mapApiToProps, cacheKey: 'siteId', // or keyProp? }

(I know I tweaked the names from your function, but I think these are more clear)

Yeah, the argument list is getting a bit long there. I'm open to some options here. The api type and the key definitely belong together, although mapApiToProps itself belongs to the component. I'd like to have some further discussion, for sure.

The api type and the key definitely belong together, although mapApiToProps itself belongs to the component

what does this mean? Are you thinking more like an (options, callback) pattern like this? I can see that working well:

export default withApiClient( { type: 'woocommerce', cacheKey: 'siteId', // or keyProp? }, mapApiToProps )( ProductForm );

I think that could work. I think keyProp would work fine for what we want. So, this object would be the specific client identifier.

ryelle · 2018-03-13T17:20:47Z

client/extensions/woocommerce/rest-api-client/actions.js

@@ -0,0 +1,41 @@
+


Nitpick, this is an extra space, and some of these files are missing the @format header

ryelle · 2018-03-13T17:21:59Z

client/extensions/woocommerce/rest-api-client/actions.js

+export function requestedAction( siteKey, endpoint, ids ) {
+	return {
+		type: WOOCOMMERCE_API_CLIENT_REQUESTED,
+		apiType: 'woocommerce',


Should this be hardcoded here? Are you making this open to any API endpoint namespace, or is this just for the WC endpoints? (following that, should the actions use a different prefix?)

Oops, you're totally right. apiType should be an argument.

And also, the actions should totally use a different prefix. I'm only prefixing them with WOOCOMMERCE_ for now while they exist under client/extensions/woocommerce.

ryelle · 2018-03-13T17:49:40Z

client/extensions/woocommerce/rest-api-client/registry/index.js

+const _apiClients = new Map();
+
+// TODO: Unit test this.
+export function registerApiType( name, apiType, apiTypes = _apiTypes ) {


What exactly is apiType? It sounds like it should be a string name, but I see above it's the createWcApi object. There's a lot of apiType* in this file, and maybe these could have better names - some documentation might help with figuring out clearer names.

Yeah, there's ambiguity here. Maybe the string should be apiName?

name is fine, but if apiType is actually a collection of "stuff", it's not a type itself - the type is "woocommerce" (in my reading). This argument could be … a client, or handlers … I don't think you need to prefix everything with API, either -- that's obvious from the file itself, and function names (which since they're used externally, can keep the Api).

So, the apiType object would represent everything one needs to use the WooCommerce API, but it wouldn't be keyed to a specific site, so what would you recommend naming that?

coderkevin · 2018-03-13T19:11:24Z

is there still the idea that part of this would be an external module?

Yes, I think so. I'm just including it all here for now so we can discuss it in one place and get it stable before adding another npm dependency.

coderkevin · 2018-03-14T15:15:40Z

After our discussion, I'm leaning towards going ahead and moving the majority of this code into a couple of new npm packages now, rather than later:

General-purpose API code.
WooCommerce API Client that depends on 1.

It would mean updating version numbers more often for both of them in the calypso package.json, but it also means we could try out the code in other places like wp-admin sooner rather than later.

timmyc · 2018-03-23T16:40:17Z

@coderkevin any thoughts regarding this pr/solution WRT the work done in WordPress/gutenberg#5253

aduth · 2018-03-23T17:43:03Z

Gutenberg's data module in general may be an interesting look, and particularly its treatment of selector resolvers, which were designed in such a way to allow substitute resolving behavior, promoting selectors as the declarative interface by which a developer expresses their data needs. The approach is a bit more simplistic than what's being explored here, having no built-in notion of data freshness or re-requests, though these may be added later. There's more discussion at WordPress/gutenberg#5219 .

This topic seems to be a popular topic of late, with Dan Abramov's JSConf Iceland talk covering a similar approach with "React Suspense" and promised fetchers.

Apologies I've not yet had an opportunity to take a deeper dive into the approach proposed here.

dmsnell · 2018-03-25T11:37:22Z

Here are some similarities to what I've been building in my still-unnamed fetchableData/futureData/failableTasks library… (https://github.com/dmsnell/redux-data-layer)

API requests deal with types of data and those types must be registered into the runtime system whenever they are needed
the wrapper function/component handles data lifecycle, freshness, etc…
API types can be created and described in one spot and even local to a component (I think…?)
the kind of data or item is referred to via some kind of string or string-like identifier
define the shape of passed data inside the wrapper

Some notable differences…

I'm not interested in storing all the data in Redux, instead relying on mutable Maps which are queried and immutably-updated. The Redux update cycle is maintained through incrementing a tick and that is the only reducer created - the combination of removing reducers and keeping storage I believe is a boon for garbage-collection pressure and separating archive-like data (post content and other large data which is normal to need to be loaded)
the wrapper uses the identifiers as values with their names to build the shape of the passed data; that is, we don't gather data there but instead rely on the registered types to try and fulfill the data based on the shape we provide
instead of starting timers I'm preferring to build a dependency graph and update that based on render calls. I haven't yet started auto-polling based on freshness when the app is idle but I will probably hit that as a single timer or interval built from the dependency graph. on each tick of that timer, just like on every render, I'll compare timestamps and see what needs updating
recognizing that API polls are almost identical in behavior to any other asynchronous fetch I'm trying to generalize so that we can wrap the "fetcher" actions in some kind of failable-task wrapper action to open up any asynchronous transport: could be a timer, a message from a WebWorker, API call, or anything else - anything with a Promise or async or XHR like lifecycle: initialize, partial, success, failure.
as a task interface I've identified request information to provide data to a component and perform information describing tasks that trigger actions like updating, deleting, or manually fetching data

that is, mine is even less coupled than the data layer currently is (see the way that my failable-task idea is actually an extraction of the WPCOM_HTTP_REQUEST type with its data, error, and progress meta)

const resourceMap = ( state, { siteId, postId } ) => ( {
    request: {
        postIsLiked: ids.postLike( siteId, postId )
    },
    perform: {
        likePost: [
            ids.postLike( siteId, postId ), // like your cache name, see repo for explanation
            () => types.likePost( siteId, postId )
        ]
    }
} )

looking forward to seeing this in a self-contained library with a minimal usage example!

alisterscott · 2018-07-24T03:42:17Z

This branch needs a rebase. Is this PR still required?

alisterscott · 2018-09-12T06:57:40Z

Closing due to inactivity - please reopen if necessary

coderkevin added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Store labels Mar 2, 2018

coderkevin self-assigned this Mar 2, 2018

coderkevin requested review from ryelle, dmsnell, a team and aduth March 2, 2018 21:56

Improve testability of registry

b98eafa

This improves the testability of the registry by making all module globals optional argument defaults.

This was referenced Mar 2, 2018

WIP: Try/wc api freshness #22537

Closed

Store: Add withWooCommerceSite HOC #21506

Closed

Store: Add wc-api readme #21274

Closed

dmsnell self-assigned this Mar 3, 2018

timmyc reviewed Mar 9, 2018

View reviewed changes

coderkevin changed the title ~~WIP: Try a modular "required data" approach for fresh API data.~~ Try a modular "required data" approach for fresh API data. Mar 13, 2018

coderkevin mentioned this pull request Mar 13, 2018

WooCommerce: Move handlers into their respective state folders. #15123

Closed

ryelle reviewed Mar 13, 2018

View reviewed changes

alisterscott added [Status] Needs Rebase and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 24, 2018

alisterscott closed this Sep 12, 2018

alisterscott deleted the try/rest-api-require-data branch September 12, 2018 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try a modular "required data" approach for fresh API data. #22961

Try a modular "required data" approach for fresh API data. #22961

coderkevin commented Mar 2, 2018 •

edited

Loading

matticbot commented Mar 2, 2018

timmyc left a comment

timmyc Mar 9, 2018

timmyc Mar 9, 2018

coderkevin Mar 13, 2018

coderkevin commented Mar 13, 2018

ryelle left a comment

ryelle Mar 13, 2018

coderkevin Mar 13, 2018

ryelle Mar 13, 2018

coderkevin Mar 13, 2018

ryelle Mar 13, 2018

coderkevin Mar 13, 2018

ryelle Mar 13, 2018

coderkevin Mar 14, 2018

ryelle Mar 13, 2018

coderkevin Mar 13, 2018

ryelle Mar 13, 2018

coderkevin Mar 13, 2018

coderkevin Mar 13, 2018 •

edited

Loading

ryelle Mar 13, 2018

coderkevin Mar 13, 2018

ryelle Mar 13, 2018

coderkevin Mar 14, 2018

coderkevin commented Mar 13, 2018

coderkevin commented Mar 14, 2018 •

edited

Loading

timmyc commented Mar 23, 2018

aduth commented Mar 23, 2018

dmsnell commented Mar 25, 2018

alisterscott commented Jul 24, 2018

alisterscott commented Sep 12, 2018

Try a modular "required data" approach for fresh API data. #22961

Try a modular "required data" approach for fresh API data. #22961

Conversation

coderkevin commented Mar 2, 2018 • edited Loading

matticbot commented Mar 2, 2018

timmyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderkevin commented Mar 13, 2018

ryelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderkevin Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderkevin commented Mar 13, 2018

coderkevin commented Mar 14, 2018 • edited Loading

timmyc commented Mar 23, 2018

aduth commented Mar 23, 2018

dmsnell commented Mar 25, 2018

alisterscott commented Jul 24, 2018

alisterscott commented Sep 12, 2018

coderkevin commented Mar 2, 2018 •

edited

Loading

coderkevin Mar 13, 2018 •

edited

Loading

coderkevin commented Mar 14, 2018 •

edited

Loading