Data: Add redux-routine package for synchronous generator flow

[aduth]

Alternative to #7546, #6999

WIP: This pull request is nearly complete, but tests in packages/data need to be updated to account for the new options.

This pull request seeks to introduce a new @wordpress/redux-routine package, used to create a generator-based coroutine middleware for Redux. The example in the included README.md should demonstrate the intent:

import { combineReducers, createStore, applyMiddleware } from 'redux';
import createRoutineMiddleware from '@wordpress/redux-routine';

const middleware = createRoutineMiddleware( {
	async FETCH_JSON( action ) {
		const response = await window.fetch( action.url );
		return response.json();
	},
} );

function temperature( state = null, action ) {
	switch ( action.type ) {
		case 'SET_TEMPERATURE':
			return action.temperature;
	}

	return state;
}

const reducer = combineReducers( { temperature } );

const store = createStore( reducer, applyMiddleware( middleware ) );

function* retrieveTemperature() {
	const result = yield { type: 'FETCH_JSON', url: 'https://' };
	return { type: 'SET_TEMPERATURE', temperature: result.value };
}

store.dispatch( retrieveTemperature() );

Atop this, it introduces a new controls option to the registerStore API which automates the creation of this middleware for stores created using @wordpress/data. In the process, it also exposes enhancer introduction (#7417), though I have intentionally chosen to not (yet) document this.

These controls can be used by either actions or resolvers. I've adapted a few existing resolvers in @wordpress/core-data as a proof-of-concept.

Testing instructions:

Verify there are no regressions in the request of resolved core data. Notably, this includes theme supports and the authors dropdown.

Ensure unit tests pass:

npm test

[youknowriad]

I think the control should also receive a function to raise errors and sometimes you want to avoid the recursion and just yields the value directly.

Question: Does it support yielding generators inside generators. I think this should be built-in to encourage composition?

[aduth]

I think the control should also receive a function to raise errors and sometimes you want to avoid the recursion and just yields the value directly.

Could potentially have it so that the control could reject its promise. I'm not sure what impact this should have though; I wanted to stay as unopinionated as possible, leaving this pattern to user-space.

[aduth]

Question: Does it support yielding generators inside generators. I think this should be built-in to encourage composition?

I expect it should work out of the box, yes, since it's just a middleware. Should have a unit test for this though.

[youknowriad]

Could potentially have it so that the control could reject its promise. I'm not sure what impact this should have though; I wanted to stay as unopinionated as possible, leaving this pattern to user-space.

But to stay unopinionated we should offer a way to gen.throw inside controls. It could be with promise rejections or by passing resolve/reject as callbacks to the controls. The latter has the advantage of keep the resolution synchronous if we don't need Promise (related to the issue where just using Promises with sync promises Promise.reject makes the code async directly)

[aduth]

The latter has the advantage of keep the resolution synchronous if we don't need Promise

This should also exist in the current implementation by just returning a non-Promise, and is tested as such.

gutenberg/packages/redux-routine/src/test/index.js

Lines 63 to 76 in 4045905

	it( 'assigns sync controlled return value into yield assignment', () => {
	const middleware = createMiddleware( {
	RETURN_TWO: () => 2,
	} );
	const store = createStoreWithMiddleware( middleware );
	function* createAction() {
	const nextState = yield { type: 'RETURN_TWO' };
	yield { type: 'CHANGE', nextState };
	}
	store.dispatch( createAction() );
	expect( store.getState() ).toBe( 2 );
	} );

[aduth]

Okay, so I see how this is handled in rungen is to throw as an error when a "reject" state is reached, allowing the developer to handle as try / catch in their action creator. Seems like a reasonable approach.

[youknowriad]

Why the handling of Promises is built-in into the runtime, it should be just another control.

[aduth]

Why the handling of Promises is built-in into the runtime, it should be just another control.

Promises are the internal mechanism for the delay, the equivalent of rungen's next function. I don't see how it's possible to not have this exist the way it does.

To be clear, this doesn't enable a developer to yield promises from their action creator. This only enables a developer to return a promise from the implementation of their control.

[youknowriad]

I guess it's related to that #8096 (comment)

[youknowriad]

I'd personally be in favor of using rungen just because it already handles all what we need:

• recursion
• raising errors when necessary
• I'm fine with simplifying the controls API and making it "action type based" like in this middleware, this should be easy to do with rungen you just wrap the given controls in the middleware to rewrite them in the expected API.

[youknowriad]

Not certain how to do it yet in this PR, but in my other alternative PRs, I extracted all the resolution mechanism (the runtime of the generators) into a runtime.js file to handle all the use cases. The result in this file is just something like:

startResolution(...)
runtime( fullfillment, () => {
  endResolution()
} )

I think it makes this file a bit more clear

[youknowriad]

I've been thinking about this.

While I prefer using rungen because it already has a lot of things ready for us (errors, composition...), I think it's valuable as is and I'd really move forward with those async actions, so let's try to get this in and we can refine with usage (it's always better like that :) )

[aduth]

I think part of my hesitation is maybe a lack of understanding of what rungen is doing / offering, and feeling that it's doing too much or otherwise enabling footguns, when the only thing we need is the bare minimum of plain action objects associated with a continuation procedure. Whether that's mapped to action types vs. controls receiving all actions and deciding themselves to act upon, I don't feel too strongly.

I'm going to iterate a bit more on this hopefully ahead of your tomorrow for feedback.

[aduth]

Incoming brain dump...

Whether that's mapped to action types vs. controls receiving all actions and deciding themselves to act upon, I don't feel too strongly.

In fact, one thing I didn't like about the original implementation here was that controls were defined by action type strings, whereas a resolver is mapped by the name of its associated selector (i.e. why not map by action creator name for consistency?).

Another option might be for createRoutineMiddleware to create just a single handler which decides whether to operate on the incoming action. It would then be reasonable to expect multiple createRoutineMiddleware to be defined for a single store.

// Before

const middleware = createRoutineMiddleware( {
	async FETCH_JSON( action ) {
		const response = await window.fetch( action.url );
		return response.json();
	},
} );

// After

const middleware = createRoutineMiddleware( async ( action ) => {
	if ( action.type === 'FETCH_JSON' ) {
		const response = await window.fetch( action.url );
		return response.json();
	}
} );

Though as implemented, this might have the negative consequence of making everything handled asynchronously (by virtue of the async function keyword). The function could be rewritten to avoid async, but then it'd lose some of its quality-of-life advantages.

Another idea might be to implement it as something like Lodash's _.cond, a tuple where one function returns a boolean indicating whether the action is to be handled.

const middleware = createRoutineMiddleware( [
	[
		( action ) => action.type === 'FETCH_JSON',
		async ( action ) => {
			const response = await window.fetch( action.url );
			return response.json();
		}
	],
	[
		( action ) => /* ... */,
		( action ) => /* ... */
	],
] );

This isn't a very obvious interface, however. It could be expressed as an object of isMatch / handle (or similar), but then it becomes quite verbose.

After having thought on this some more, I'm trying to decide if what's been proposed here is a bizarre amalgamation of a middleware which would be better represented by a separation between handling generators vs. the asynchronous continuation, and whether each "control" is itself most easily expressed as a middleware (even if the developer is providing these themselves).

It's reached the end of my day, and these ideas are becoming fuzzy to me, so I'll have to revisit it when I'm fresher in the morning.

[aduth]

I've rebased this to resolve conflicts.

I've also made the following changes:

• Rejecting during a control promise will throw to the generator, allowing it to handle via try / catch
• Reimplemented registry controls as a data plugin

As of now, I've backed out deprecation of async generators. I want to think more on how to approach resolvers generally, in the sense that they should ideally just pass through to store.dispatch directly. The thinking here is avoiding the base registry needing to know anything about particular return values of resolvers, and rather let middlewares handle this themselves. This is complicated by the need to have start / end detection. I want to revisit this early next week.

Alternatively, we could push forward with this as-is without any usage, or a simpler example usage that doesn't interact with resolvers but could still benefit from the generator pattern (something in effects.js, for example). I'm open to suggestions.

[youknowriad]

LGTM 👍

I'm ok merging and trying to use in effects

[youknowriad]

Question: What do you think about the inconsistency of the actions behavior?

• actions with controls returning something defined by the control
• actions without controls returning the action itself or undefined?

[aduth]

Are you suggesting we should promote yield actions.setPrice( item, price ) for consistent way of causing the dispatch?

I'm not really sure there's an inconsistency here, in that the actions.fetchFromAPI itself is still just a plain action object; it's the act of yielding it which transforms its result into being assigned into the variable or return statement.

[youknowriad]

I'm just thinking that maybe fetchFromAPI is not really an action and might be declared separately. controls.fetchFromAPI

[aduth]

As I see it, the semantic purpose for an action is to express an intent. To that end, we traditionally consider its use in the context of dispatching within the store, but I don't see it as being fundamentally different from what we're proposing with controls, where the intent is processed as defined by the continuation procedure via the middleware.

It kinda speaks back to my thoughts at #8096 (comment), where there are multiple things going on here: namely the handling of generator and the potentially-asynchronous continuation. I'm led to think that they are complementary for the purposes we're using them for, but also that it leads to open questions on:

• Is it okay to have a generator action creator which doesn't cause any asynchronous continuation to occur?
• Is it okay for a control to be used without an attached asynchronous behavior (i.e. returns synchronously to assign / return on the yield)?

Both seem like implementation details that the action creator needn't be concerned with. It could be asynchronous, or it could not. From the developer's perspective, it's important that it's consistent in how it's used: yielding can assign a value, whether that's assigned asynchronously or not. It's a nice bonus that it provides a solution for a common use-case (multi-dispatch).

Thinking on how this is at all different from effects, the one thing that stood out to me is that we quickly turned to effects as they were the only option to do either asynchronous or multi-dispatch for a while. And once something was converted to an effect, it became that much more convenient to stay in the effect handler to perform the myriad of behaviors associated with an action. By contrast, with the routines / controls, it establishes a simple and obvious pattern to temporarily escape out of the flow from within the action creator itself in an isolated fashion.

[aduth]

Note: I passed this argument for consistency with how we register other scripts, but it's not obvious to me why we need $in_footer assigned for these registered scripts.

[aduth]

Should probably add mention of this requiring the controls plugin to be used.

[aduth]

Added in a675f49

[aduth]

Question: Needs to distinguish on asynchronous generators?

[aduth]

Improved accuracy in c1f1c35

[youknowriad]

I made some small fixes to fix the unit tests. I'm going to merge this ASAP to try it in follow-up PRS :)

[gziolo]

@babel/runtime should be listed here because tranpiled code contains references to it. Example:

import _Promise from "@babel/runtime/core-js/promise";

[gziolo]

It's enabled by default, there is no need to include it.

[aduth]

Thanks @gziolo . I'll whip up fixes shortly!