Framework: Fetch categories by common terms resolver

[aduth]

This pull request is part of a series of pull request targeted at more complex use of selector resolvers introduced in #5219, namely toward paginated hierarchical terms picker.

This pull request seeks to refactor the category resolver implemented in #5219 to generalize its use toward terms fetching. There are no changes to the public interface for consuming categories, and it's expected that a getCategories selector will remain long-term as a boon to developer usability. The changes here enhance the existing getTerms selector on which categories extends to support resolution of other terms. This includes necessary changes to allow access to the REST base mappings within the core data module (via wp.apiRequest.settings).

Implementation notes:

It would be nice if we wouldn't need a standalone getCategories resolver, considering that the getCategories selector calls the getTerms selector, there should only need to be a resolver for getTerms. However, because selecting within a module's own selectors does not flow through the data module's abstraction, it does not trigger these resolvers automatically. While disappointing, this is a relatively minor consideration.

Testing instructions:

Verify there are no regressions in the display of categories block.

Verify there are no regressions in the behavior of withAPIData, whose provider has been slightly revised here. Example: Category picker.

[youknowriad]

I wonder if we could avoid the apiRequest.settings by fetching the rest base from /wp/v2/taxonomies or something? A resolver calling another resolver? Any thoughts on this.

Just trying to avoid as much globals as we can

[youknowriad]

Thinking more about this, getTaxonomy could be a selector with a side effect.

const { rest_base: restBase } = yield select( 'core' ).getTaxonomy( taxonomy ) which will call the side effect only once.

[youknowriad]

I guess we need a resolve here because we want to wait for fulfillment

[aduth]

That might work. We'd definitely want to bootstrap this data, to avoid the additional request occurring in the client before being able to resolve terms. Bootstrapping support might be something we want anyways.

Part of the goal in assigning wpApiSettings into apiRequest was to avoid the introduction of another global, since apiRequest is already faked into module form as @wordpress/api-request.

[youknowriad]

wp.apiRequest.settings is definitely better than wpApiSettings or wp.api.*, we'd have a unique root global dependency wp.apiRequest. If you think we can try the resolver approach separately, I'm fine with that.

[aduth]

Another issue with separate resolvers is that because the data module memoizes per function, calling getTerms( 'category' ) and getCategories() will cause two network requests, despite being for the same resource.

To your previous point, maybe this is where we ought to go through the select API to resolve terms for the getCategories resolver. This would also mean we'd probably not need (or want, in order to avoid this scenario) to provide state as an argument of the resolver.

[aduth]

One other option, which could also help address another issue I'm encountering†, is to introduce the isFulfilled function discussed in #5219.

Previously we considered it being an object. I'm wondering also if it could just be a property of the resolver function.

export async function* getTerms( state, taxonomy ) {
	yield setRequested( 'terms', taxonomy );
	const restBase = getTaxonomyRESTBase( taxonomy );
	const terms = await apiRequest( { path: '/wp/v2/' + restBase } );
	yield receiveTerms( taxonomy, terms );
}

getTerms.isFulfilled = ( state, taxonomy ) => {
	// return hasRequestedTerms( state, taxonomy );
	return state.terms[ taxonomy ] !== undefined;
};

† I'm trying to refactor the HierarchicalTermsSelector component to query from core data. To do this, I'm inclined to add a new optional options argument to the selector for things like perPage, orderBy, etc. However, this is an issue for memoization, since getTerms( 'category', { perPage: 100 } ) will never memoize (the arguments are compared by reference, and here we create a new object each time). There are various ways we could solve this (constant reference to the query object, memoize on arguments deeply), but the above function could work and avoids imposing any considerations on the consuming developer.

Even if this was implemented another way (e.g. separate selector like getCommonlyUsedTerms), it seems inevitable that eventually someone will create a selector accepting an object which would fail to cache its having been resolved.

[aduth]

Or, if the resolver has access to state, it can simply abort itself if no action is necessary.

[youknowriad]

I'd love if we avoid the isFulfilled API but at the same time I don't see how would you solve the issue even with state access (when the query is in flight) unless you memoize by deeply comparison.

Maybe it could be optional, if provided use it, if not use memoization

[aduth]

I don't see how would you solve the issue even with state access (when the query is in flight)

Since we synchronously yield setRequested in the terms resolver, all subsequent resolutions should have awareness that the request has been made, even if a request is currently in flight.

This is the idea anyways. I'm having trouble with this in practice, where each call to getTerms has empty requests state. Thinking there might be something off with yielding / resolution occurring in expected order.

[aduth]

It appears to be that since the asynchronous iterator of registerResolvers operates effectively as an array of promises, and since promises don't execute until after the current call stack, both calls to the resolver trigger the setRequested before either has a chance to dispatch to the store.

Related:

• https://medium.com/netscape/async-iterators-these-promises-are-killing-my-performance-4767df03d85b#997c
• https://promisesaplus.com/#point-34

[youknowriad]

@aduth Funny, that's exactly why I created rungen in the first place. redux-saga was using promises initially and I wanted to use callbacks so synchronous effects are run synchronously :). So I believe it's doable.

[aduth]

So I believe it's doable.

Is it a simple change? Or are we talking introduction of wrapping functions and custom vocabulary?

[youknowriad]

Since we're not supporting a lot of yieldable values, I believe it should be simple. I haven't looked in detail to the way you implemented the engine behind the generators syntax but I don't think it's a complex task, just replace the promise usage, using simple callbacks (success, error)

[youknowriad]

@aduth

Here's the whole rungen engine implementation https://github.com/youknowriad/rungen/blob/master/src/create.js#L4-L36

We could use exactly the same implementation and just replace the controls array here https://github.com/youknowriad/rungen/blob/master/src/create.js#L5 with an array containing only our action control.

A control is basically the behavior saying if we have this kind of yielded value, do this behavior and return this value (See the readme of rungen to undersand better).

We only support one unique control in wp.data the action control.

[danielbachhuber]

@aduth @youknowriad Is this PR still relevant?

[aduth]

Yes, this still has value, but needs to be brought up to date.

Specifically, we can leverage the isFulfilled API which has since been added (#6084) and enhanced with deep arguments memoization (#6360) as mentioned here previously in #5826 (comment) and #5826 (comment).

At least this can serve as an adequate solution for our needs here. The complexities of handling asynchronous dispatching may still warrant more investigation separately.

[aduth]

I believe this is no longer required with the more recent entities implementation (#6939).