Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data: Add redux-routine package for synchronous generator flow #8096

Merged
merged 9 commits into from
Aug 7, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 20, 2018

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

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress npm Packages Related to npm packages [Package] Data /packages/data [Package] Core data /packages/core-data labels Jul 20, 2018
@aduth aduth requested a review from youknowriad July 20, 2018 21:27

const control = controls[ nextAction.type ];
if ( typeof control === 'function' ) {
const routine = control( nextAction );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 );
} );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if ( routine instanceof Promise ) {
// Async control routine awaits resolution.
routine.then( ( result ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


step( action.next().value );
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( isActionLike( maybeAction ) ) {
store.dispatch( maybeAction );
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Member Author

aduth commented Jul 30, 2018

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
Copy link
Member Author

aduth commented Jul 30, 2018

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 aduth force-pushed the add/redux-routine-package branch from 4045905 to fa6a273 Compare August 4, 2018 23:23
@aduth
Copy link
Member Author

aduth commented Aug 4, 2018

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.

@aduth aduth force-pushed the add/redux-routine-package branch from fa6a273 to 5d9b7ad Compare August 4, 2018 23:27
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I'm ok merging and trying to use in effects

dispatch( 'my-shop' ).setPrice( item, price );
* getPrice( state, item ) {
const path = '/wp/v2/prices/' + item;
const price = yield actions.fetchFromAPI( path );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

gutenberg_url( 'build/redux-routine/index.js' ),
array(),
filemtime( gutenberg_dir_path() . 'build/redux-routine/index.js' ),
true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


The `resolvers` option should be passed as an object where each key is the name of the selector to act upon, the value a function which receives the same arguments passed to the selector. It can then dispatch as necessary to fulfill the requirements of the selector, taking advantage of the fact that most data consumers will subscribe to subsequent state changes (by `subscribe` or `withSelect`).

### `controls`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in a675f49

* @return {boolean} Whether object is a generator.
*/
export default function isGenerator( object ) {
return !! object && typeof object.next === 'function';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Needs to distinguish on asynchronous generators?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved accuracy in c1f1c35

@youknowriad
Copy link
Contributor

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

@youknowriad youknowriad merged commit 1949e3a into master Aug 7, 2018
@youknowriad youknowriad deleted the add/redux-routine-package branch August 7, 2018 09:32
},
"main": "build/index.js",
"module": "build-module/index.js",
"dependencies": {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

*/
import createMiddleware from '../';

jest.useFakeTimers();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aduth
Copy link
Member Author

aduth commented Aug 7, 2018

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

@mtias mtias mentioned this pull request Aug 13, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript npm Packages Related to npm packages [Package] Core data /packages/core-data [Package] Data /packages/data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants