Try Selector Side Effects and withData HoC

[youknowriad]

This PR is not polished yet as it's just an exploration at the moment.
It introduces two new APIs:

• When registering selectors we can attach side effects to each selector like this:

wp.data.registerSelectors( { 
  mySelector: {
    select: ( state, ...args ) => state.something // the real selector
    effect: ( ...args ) => { // do something with the args }
  }
} );

• a new Higher Order Component you can use to resolve data (run selectors and their side effects)

wp.data.withData( ( resolve ) => ( {
  categories: resolve( 'core' ).getCategories()
} ) );

the withData behaves closely to the withSelect HoC but also runs the side effects attached to each selector. The other difference is that the select function passed as arg to the withSelect HoC returns the real result of the selector when called while the resolve function passed as arg to the withData returns just a descriptor (or resolver) used to select the data an trigger the side effect when needed.

---

An example implementation has been tried in the newly introduced core-data module. This module register a selector to fetch categories and is being used in the categories bloc to test it.

[aduth]

Had a chat with Riad about this earlier. In summary, if it's at all possible, I would love if this data resolution was transparent within existing APIs, at least as far the data consumer is concerned.

Some rough pseudo-code for how I might imagine this working:

registerSelectors( 'core/core-data', {
    getCategories( state ) {
        return state.categories,
    },
} );

registerResolvers( 'core/core-data', {
    getCategories() {
        return wp.apiRequest( { path: '/wp/v2/categories' } )
            .then( ( categories ) => {
                dispatch( 'core/core-data' ).receiveCategories( categories );
            } );
    },
} );

// Will automatically call resolver:
select( 'core/core-data' ).getCategories();

To avoid unnecessarily fetching data on repeated calls to the selector, we could either infer that data exists by having a convention on returning undefined from a selector if and only if data needs to be requested. Alternatively, we could make this part of the resolver registration API:

registerResolvers( 'core/core-data', {
    getCategories: {
        isFulfilled: ( selectorResult ) => selectorResult !== undefined,
        fulfill() {
            return wp.apiRequest( { path: '/wp/v2/categories' } )
                .then( ( categories ) => {
                    dispatch( 'core/core-data' ).receiveCategories( categories );
                } );
        },
    },
} );

We would need also to avoid duplicate fulfillment requests while one is already in-flight. I'm not sure exactly how this would look, but I might imagine the wrapper applied to the original selector by registerResolvers could track the promises returned by fulfill (keyed by arguments passed to selector), attaching any future fulfillment requests via Promise#then. Roughly:

const promises = {};

if ( ! isFulfilled ) {
    const key = serialize( args );
    if ( ! promises[ key ] ) {
        promises[ key ] = fulfill();
    }
}

return promises[ key ].then( () => /* ...trigger another selector call? */ );

A few challenges I've not yet been able to work through:

• How does the withState mapping get called again after resolver has been fulfilled?
• How does data get refreshed? (Does it need to be?)

[youknowriad]

Update the PR and implemented something close to what's proposed by @aduth
There's a different in how isFulfilled work. Instead of providing the result of the selector as an arg, I'm providing the state and the args. The idea is that sometimes we want to always return an array from the selector even if the state is still undefined (My example getCategories matches this use-case).

Also, I was able to avoid running the same request over and over again while it's inflight by using memize for the fulfill funciton. That way if the args are equivalent we avoid calling the method over and over again.

The issue with this could be the memory because the cache will keep growing that's why I have a todo in my code, I'm proposing adding an API like this to memize cc @aduth :

memoizedFunction.clearCacheItem( ...args )

to remove an entry from the cache and it will be used once the selector is fulfilled.

[aduth]

I don't think we need to do anything special here. If fulfill returns a promise, we can just then to it because it works, whether or not the request has already been completed.

See example: http://jsbin.com/qetusew/edit?html,js,console

[aduth]

I guess it becomes a question of whether, if we want separate isFulfilled and fulfill, whether we need to keep around the cached value for fulfill, since we'd expect future calls to isFulfilled to return true.

Where this could be useful is if we established a convention of the selector or state returning a value indicating that it is not fulfilled: null or, more likely undefined, since we may want to express meaningful emptiness via null.

Edit: I see this is what you meant in your comment by:

The idea is that sometimes we want to always return an array from the selector even if the state is still undefined (My example getCategories matches this use-case).

My intuition is that we don't want to do this. If it's not available, it shouldn't return an array. A convention around this could also be a useful signal to the components to know whether they need to display some loading indicator, rather than having to pass this as a separate prop.

[youknowriad]

The idea is that we don't want to call fulfill if it's already in-flight or fulfilled.

1- If it's already fulfilled, we have a dedicated API for this isFulfilled
2- If it's already in flights, we need to cache a value saying that fulfilled has already been called, we can do so by storing a map [ args ] => promise or we can just rely on a memoization library doing exactly that, avoid calling a function again if the args are the same.
I preferred the second option using memize because it seemed simpler and memize is already well optimized.

Now, when the selector is fulfilled, we may want to clear the memoized value (or the cached promise) because it's not necessary anymore. That's what my TODO comment is about.

[youknowriad]

But I agree that returning a promise from fulfill is a good convention to have even if we don't use right now, it could be useful later for other purposes.

[aduth]

I'm curious how this might fit into the larger story of reducing these dependencies to the API directly, where fulfillment is merely a set of signals, or could otherwise be swapped with a specific implementation.

• Could they return actions which are observed and answered by the implementing interface?
• Could an implementing interface easily replace these resolvers with their own if they so choose?

[youknowriad]

So the question is whether this core-data module I'm adding here is considered the data layer of Gutenberg in which case it should rely on swappable API layer or Should we consider this module as the adapter it-self in which case it should be entirely replaced by another implementation. Think my-custom-data module replacing core-data when Gutenberg is used in other contexts.

Honestly, I don't know the answer here, I'm still on the fence.

[mkaz]

I see it as the later, this module as the adapter with the whole thing being replaced.
So I could create my own data module which defines getCategories() and does whatever it needs and returns the data Gutenberg needs.

I think another API layer on top assumes too much on where the data might be coming from.

[aduth]

How do we signal to the original consumer that the value is now ready after this completes?

[youknowriad]

Since the selector is state based, the resolver will update the state at somepoint which will trigger a subscribe and rerender the consumer.

[youknowriad]

After a discussion with @gziolo around the need of isFulfilled, I updated this PR to avoid it entirely and just rely on the fact that fulfill is memoized which also makes the implementation so simple.

[gziolo]

I discussed this idea with @youknowriad a little bit. In general, I love the direction and the proposed way of registering resolvers. It's all great and we should definitely continue this work to further unify data access. My only concern is this part:

isFulfilled: ( state ) => state !== undefined,

I would prefer to have it auto-handled by the data module. I'm not quite sure what is the best way to tackle it, but speaking myself we should look for alternative solutions here.

We also need to keep in mind that fulfill needs to be able to reset its state when it errors, so it could re-try to fetch the data. This is where memoization can get tricky. We should explore it further. I don't know how it compares with the current implementation of withApiData though. Maybe it isn't it an issue at the moment.

[aduth]

We need to establish some conventions around module names, both internally and as recommendations for plugins.

Elsewhere we prefix with a core/ namespace and align the remainder of the name with the top-level folder. Here, that would mean core/core-data.

Obviously there's some ugliness in the repetition here. @mtias mentioned this at one point as well. My first thought was that we could drop the namespace (or make optional) for core modules, similar to what we do with blocks.

But then for plugins, we need to make certain that they are namespaced. But for many plugins, they may only have a single store. What are they to call their stores? yoast/yoast ?

While verbose, at least the core namespace enables plugin authors to avoid their own, while avoiding potential name conflicts.

Whatever we decide, we should also be mindful of consistency with other "namespace + name" patterns.

[youknowriad]

I think we could have a special case here because this core-data module will contain all "core" data, it could even be included in the data module itself if we didn't want to keep the data module generic. But happy to follow any consensus here.

[aduth]

I love how this file has been refactored 👍

[aduth]

Should we use the createStore API? Should resolvers be integrated into this API?

[youknowriad]

True, it was not there yet when I started the PR ;)

[aduth]

Minor: Function property shorthand is more compact than arrow function:

fulfill: () => {
fulfill() {

[aduth]

Does fulfill need to be a property of the object? Are we anticipating more properties? Couldn't the function just be the value of the getCategories key?

[youknowriad]

Are we anticipating more properties?

I thought about this and I think we should keep it as an object to allow extra properties in the future. I can think of a TTL property for example.

[gziolo]

There a few alternative ways to do it if we need to:

• function when no additional properties or object otherwise - type detection in the background
• higher-order function which adds additional behavior
• return value from the fulfill implementation

Just saying that we don't need to default to the object. I'd be in favor of starting with the function and change it if we find it too limiting.

[aduth]

Yeah, I think setting it as a function is still forward-compatible with the patterns @gziolo outlined, and a nice simple usage when you don't need more complex options.

function normalizeResolver( resolver ) {
	if ( typeof resolver === 'function' ) {
		return { fulfill: resolver };
	}

	return resolver;
}

[aduth]

• Maybe fine as a demo / proof-of-concept, but this reducer shape certainly won't scale to accommodate more data.
• We should want some documentation when we do flesh it out.

[aduth]

What are we spreading?

[youknowriad]

isFulfilled :) or Right I removed it :P. I guess we could leave to accommodate future options but I'll just remove, it's easy enough to add later.

[aduth]

If both logic paths return result;, maybe it'd be simple to move fulfill() into the condition above as negated?

if ( resolver ) {
	resolver.fulfill( ...args );
}

return result;

[aduth]

Minor: Good use case for _.get†

const resolver = get( resolvers, [ reducerKey, key ] );

† in that it arguably helps readability

[aduth]

This seems particularly heavy for an operation which we're assuming to occur very frequently (select). Could the selectors be augmented once as part of registerResolvers instead?

[aduth]

We should just eliminate these lists of top-level folders where they're not strictly necessary. This is already inaccurate because it doesn't include viewport. A maintenance nightmare 😬

[gziolo]

It might be a good idea to move all modules to a subfolder and include README there and reference it everywhere we want to leave a note about other modules. I think it would be more useful than that. We should consider that when we start publishing modules to npm to have everything grouped together. This would also add a clean separation between production code and everything else.

[youknowriad]

@gziolo I like it, that's how I do it in GCF https://github.com/youknowriad/gcf/tree/master/scripts

[aduth]

Yes, we need to consolidate each of these horrendous lists of modules. Something like a Lerna approach of folder structure would be fine. I was even thinking in the immediate-term just a file modules with newline separated list, and everything that depends on it must read from it (single place to update).

[youknowriad]

Let's discuss this approach, if you all think, this approach is good enough, I'm happy to consolidate, add some unit tests...

[gziolo]

❤️

[gziolo]

I like the public API (minus fulfill layer ), so you definitely have my vote to proceed 👍 We can discuss at the Core JS meeting today.

[aduth]

I was iterating on this some today. I simplified the registration to assign the resolver function directly as the object property's value, as discussed in #5219 (comment).

I was starting to look at updating the documentation for the data module, and it dawned on me that as implemented, we will pass the arguments of the selector, but not state to the resolver. Is that what we'd expect, or should we also pass state to align directly with the signature of the selector?

registerStore( 'my-shop', {
	// ...

	selectors: {
		getPrice( state, item ) {
			const { prices, discountPercent } = state;
			const price = prices[ item ];

			return price * ( 1 - ( 0.01 * discountPercent ) );
		},
	},

	resolvers: {
		getPrice( /* state? , */ item ) {
			return apiRequest( {
				path: '/wp/v2/prices/' + item,
			} ).then( ( price ) => {
				dispatch( 'my-shop' ).setPrice( item, price );
			} );
		},
	},
} );

To implement this, we'd need to either apply to the selector after state has been pre-applied as an argument, or manually pass state into fulfill. An idea to achieve the former pretty easily is to apply a filter for the selector result, to which resolvers could simply hook. This seemed like an easy way to expose filterability of selectors. But the more I thought about it, the more I was worried about how this could be misused, or cause unpredictability of selector results. The one area I thought it could be useful - overriding the isEditedPostDirty of editor to account for edit-post's hasActiveMetaBoxes (so the save button is always active) - made this illustration more real. We'd not want to override the selector for all usage, just specifically for what edit-post is using it for. Ideally we'd have some easy pattern for composing selectors from other stores, so we could have a custom selector for edit-post which extends the result of isEditedPostDirty. Technically we could just call select from within the selector, but it's not clear whether this is the cleanest approach.

I wondered too about how we'd want to suggest reusing action creators in the callback of the resolver's promise. In our core-data example, we dispatch directly without an action creator. But I don't think this is the usage we'd want to promote, if we're otherwise suggesting dispatching via defined action creators as the main mechanism for manipulating state. We could, as in the example above, call dispatch to invoke a registered action creator. Another option is exposing action creators on the store instances itself, so we could call store.actions.setPrice via the return value of registerStore. It would be nice if we could do some sort of promise resolution to an action creator, but as defined currently in the example this wouldn't be very easy to achieve. We'd need to move the action creators out of the inline registerStore call to reuse:

function setPrice( item, price ) {
	return {
		type: 'SET_PRICE',
		item,
		price,
	};
}

registerStore( 'my-shop', {
	// ...

	actions: {
		setPrice,
	},

	resolvers: {
		getPrice( /* state? , */ item ) {
			return apiRequest( {
				path: '/wp/v2/prices/' + item,
			} ).then( ( price ) => {
				return setPrice( item, price );
			} );
		},
	},
} );

Which is maybe not so bad, since outside of the most simplest examples, we'd probably want to define action creators as standalone functions anyways (not inlined to the registerStore options object). In the above case, if the return value of a resolver is a promise which itself resolves to an action, we'd infer to dispatch that action object.

[gziolo]

core-data still misses README file.

[gziolo]

In this case we don't even need to specify an action creator as it seems it shouldn't be executed in other places. However, it might make sense to always create a corresponding action creator to make it easier to extend. If someone wants to override this resolver, they will need to have a simple way to dispatch it anyways.

[aduth]

However, it might make sense to always create a corresponding action creator to make it easier to extend.

I also think it sets a good precedent for those who learn by copying referencing core code.

[aduth]

I've made quite a few revisions here, but I think it's in a good shape to merge now.

I experimented with leveraging async iterators to simplify resolver implementations. These are a new feature ratified as part of the ES2018 specification. As yet, we've not even made use of async / await (ES2017), so this is certainly going to be a new concept for many, but one which I think lends to very readable resolvers once understood:

export async function* getCategories() {
	yield setRequested( 'terms', 'categories' );
	const categories = await apiRequest( { path: '/wp/v2/categories' } );
	yield receiveTerms( 'categories', categories );
}

Of course, it's not necessary to write resolvers this way. Via internal normalization, they're very flexible in the shape they can take:

• A function returning undefined
• A function returning an action object
• A function returning an array of action objects
• A function returning a Promise resolving to an action object
• A function returning a Promise resolving to an array of action objects
• A generator function yielding action objects
• A generator function yielding Promises resolving to action objects
• An async generator function yielding resolved action objects

So a more "traditional" implementation might look like:

export function getCategories() {
	return apiRequest( { path: '/wp/v2/categories' } ).then( ( categories ) => {
		return receiveTerms( 'categories', categories );
	} );
}

As can be seen in the above snippets, I've also refactored the categories implementation to operate on terms generally, since while we should probably want to expose these higher-level selectors, the need for underlying terms management will ensure consistency with future additions (e.g. tags, custom taxonomies).

I included an isRequestingCategories selector to set a standard for how we can architect requesting state, since I tend to dislike the verbose START / SUCCESS / FAILURE action type pairings. First in ffb0760 with a requested state, then later as part of refactoring to generic terms dbdf02c. The basic idea is that we know a fetch is in progress if the current data state is unassigned and we know that the request has at one point in time been initiated.

Documentation has been added for Core Data, and resolvers example added to the data module's README.md . As noted in #5219 (comment), I still want to make further improvements here, but will do so separately. A potential conflict here is the aligning of names between resolvers and selectors, which can occasionally cause variable naming conflicts.

[aduth]

@youknowriad had raised some reluctance about the use of async generators, notably for (a) potentially blocking future enhancements to support yielding more than just dispatching actions and (b) being confusing for developers. I feel fairly comfortable that future enhancements, if any were to exist, would not be incompatible with this implementation. While isActionLike is indeed duck typing, it aligns with what Redux imposes as a requirement for an action (string type). The idea of supporting other yield-ables isn't certain, and I don't think would be too difficult to identify. Certainly not worth muddying the API with saga-like method vocabulary, in my opinion.

I agree that generators in general are an unfamiliar concept, and further with async generators being a brand new feature (ES2018) are even further unknown. It may be that while generators are not inherently complex, they are so unlikely to be encountered such that seeing them in use here may have the potential for confusion. That said, when understood they make for an elegant API for asynchronous dispatching, and are totally optional for developers to leverage in resolvers.

With Riad's blessing and a desire to start iterating on the further enhancements here (porting existing withAPIData use and stress-testing with complex paginated resources like a functioning author/category picker), I'm going to merge this in its current form.