Data: Refactor resolver fulfillment / request progress as data state

[aduth]

Related: #6395

This pull request seeks to implement a new resolver state in the data module itself. This is used for...

• ... Tracking whether fulfillment for a resolver has already occurred for a given set of arguments.
• ... Replacing the need for dedicated requests state in other modules.

Specifically, this should help simplify implementation of core-data, both in master and as proposed in #6395, as we'd not need to track whether a request is in progress, as it can be determined from the current progress of the selector resolver.

Implementation notes:

• This is the first instance of using select within a selector, which may be subject to debate or at least a decision on consistent approach. While it feels less "pure", it does enable (albeit currently awkward) selector stubbing which can alleviate some existing issues with providing a state shape to a selector satisfying the implicit dependencies of its composed selectors (example of how these revisions can require cascading changes to many dependent selectors).
• EquivalentKeyMap was updated to 0.2.0 to take advantage of the need for cloning the state value (see changelog)

Testing instructions:

Only the Categories block makes use of the impacted data. Verify there are no regressions in the load and display of categories in the block. The "load" stage is particularly affected, and a spinner should be shown while the request is in progress.

[youknowriad]

I think this is a great addition to the data module. Some questions I have

• Can we automatically generate the isResolving selector for all registered stores

• Do we need an isResolved selector as well?

• This works only if the resolver is written as a generator, should we deprecated the resolver as function?

• Also, this only works if the last expression in the generator is a yield, because if it's a promise (await) we don't know when it's finished. An alternative here would be to avoid async/await and bundle promise resolution into the generator runtime itself (yield promises + isPromiseLike) or maybe we can assume that generators would generatlly finish with a yielded action to alter state?

[aduth]

Can we automatically generate the isResolving selector for all registered stores

We can, and it might be a good option. It's a small difference, though it'd be the first instance of an auto-generated selector injected for each store, so I wasn't sure how best to document / make it clear.

Do we need an isResolved selector as well?

Is that satisfied by the included hasFinishedResolution ? What's expected to be returned by this?

This works only if the resolver is written as a generator, should we deprecated the resolver as function?

Every resolver is cast to an AsyncGenerator type, so it should be cross-compatible.

gutenberg/data/index.js

Lines 459 to 487 in 65aaac8

	/**
	* Normalizes the given object argument to an async iterable, asynchronously
	* yielding on a singular or array of generator yields or promise resolution.
	*
	* @param {*} object Object to normalize.
	*
	* @return {AsyncGenerator} Async iterable actions.
	*/
	export function toAsyncIterable( object ) {
	if ( isAsyncIterable( object ) ) {
	return object;
	}
	return ( async function* () {
	// Normalize as iterable...
	if ( ! isIterable( object ) ) {
	object = [ object ];
	}
	for ( let maybeAction of object ) {
	// ...of Promises.
	if ( ! ( maybeAction instanceof Promise ) ) {
	maybeAction = Promise.resolve( maybeAction );
	}
	yield await maybeAction;
	}
	}() );
	}

Also, this only works if the last expression in the generator is a yield, because if it's a promise (await) we don't know when it's finished.

Hmm, I'm not sure I understand. Could you think of an example, or write a failing unit test?

[youknowriad]

What if my resolver is something like this:

const myResolver = () => {
   wp.apiRequest(something).then(( data ) => {
      dispatch( 'store' ).setData( data )
   })
}

I believe in this case, there's no way to ensure isResolving and hasFinishedResolution work properly? or am I missing something?

[aduth]

@youknowriad I think it shouldn't be a problem if the last promise is either returned or awaited in the generator function.

Small sample script (requires Node 10.x):

async function sleep( duration, value ) {
	return new Promise( ( resolve ) => {
		setTimeout( () => resolve( value ), duration );
	} );
}

async function* resolve() {
	yield 'Initial';
	yield sleep( 1000, 1000 );
	yield sleep( 4000, 4000 );

	return sleep( 2000 ).then( () => {
		console.log( 'Dispatch something.' );
	} );
}

( async () => {
	for await ( const promised of resolve() ) {
		console.log( promised );
	}

	console.log( 'Done!' );
} )();

Output:

Initial
1000
4000
Dispatch something.
Done!

[aduth]

Actually, forget Node, you can just paste that into your (Chrome) browser console. Yay V8 😄

[youknowriad]

I think it shouldn't be a problem if the last promise is either returned or awaited in the generator function.

I know and that's exactly why I'm proposing to deprecate the resolvers as regular function

[aduth]

I'm confused because I don't think anything needs to be changed for it to just work, regardless of what shape the resolver takes, because of the normalization to asynchronous generator. I added more test cases in 2d63d88 to try to confirm this.

Is the issue more that a developer might "do it wrong" and not properly return their promise from the resolver?

[youknowriad]

I guess the problem for me is that I didn't think of resolvers not returning promises as "doing it wrong". I'm fine considering these as such, maybe we could just clarify it in the docs.

[youknowriad]

Let's see if we want to autogenerate this one later on.

[youknowriad]

LGTM 👍

[gziolo]

We probably should move out all methods from the index.js to avoid this kind of mocking.