Data Module: Add support for action creators as generators

[youknowriad]

I feel like this the last missing piece to the data module. A way to replace the "effects" we have.
Since we support resolvers as action generators why don't we support actions as action generators 🤷‍♂️

Questions

• The only question I have is how can we access the state from these generators. At the moment, I'm using wp.data.select directly which works but makes testing those components a bit hard.

[youknowriad]

To access the state we're calling wp.data.select directly. Another approach would be to inject a select helper somehow into the generator:

export function* autosave( select ) {
   const isSaveable = select( 'core/editor' ).isEditedPostSaveable(); // or just `select.isEditedPostSaveable()`
}

The downside of this approach is that it adds an extra argument to the action creator which is not ideal.
Maybe it's fine to rely on the global and mock the global in the tests. Thoughts?

[mcsf]

At the moment, I'm using wp.data.select directly which works but makes testing those components a bit hard. (see the refactored autosave effect)

Worth noting that this is a necessary consequence not so much of the move to generators, but of going from pure-function action creators to stateful/effectful action creators. Just pointing out the obvious — one of the ideas that Redux brought about was the limitation of responsibilities of action creators and other entities — so that we can weigh the trade-off we want to make, the upside of which I assume is to improve the developer experience of plugin authors.

[youknowriad]

Also noting that with redux-thunk, redux-saga and all alternative async handling middlewares, action creators are stateful. In this PR I'm not bringing something new, this is already a well-proven concept.

[mcsf]

Agreed, and this may be good time to try to delineate what should be available to action creators:

• If we look at thunk, common things are application state, but also any local state carried in the closure of thunks, especially when keeping track of async requests, etc. This is pretty powerful but quickly gets hard to control or make assumptions about.
• If we look at this PR and autosave, we only have (for now) application state, and that of only one store. As you pointed out in an inline comment, we could provide some helpers directly to action creators generators. What else would be the bare minimum?

[aduth]

Related Slack discussion (Core JS Meeting 2018-05-29): https://wordpress.slack.com/archives/C5UNMSU4R/p1527600900000259

[aduth]

(Mind the brain-dump 😄)

The main thing I've been disappointed in with side effects implementations, refx, redux-thunk, this one, or otherwise, is that most all fail to put the developer into the mindset of actually considering the effects to be isolated from the initiating action.

Instead, most all of our effects in the editor are written as though they were a single set of steps otherwise inseparable from the original action. If I were to guess (even speaking for myself), the mindset here is a fixation on dispatching asynchronously, not as considering actions like network requests as side effects, and certainly not to the extent of other separate effects (notices, screen reader announcements, etc.)

My hope with refx was to serve as a tool which could support...

• Potentially cascading effects.
  • Rather than a single sequence of instructions all tightly bound to the initiating action.
  • The second store argument being accepted as an edge-case, not the most common usage, especially not just for the purpose of multi-dispatching (where cascading can serve)
• Multiple separate handlers for the same action types.
  • Rather than lumping all logic into a single mega-function.
  • refx effects can be defined as (and in fact are normalized to) an array of handlers, but we're not taking advantage of this.
• Being separate from the action creator.
  • Rather than being considered as the action creator itself (thunk, this pull request).
  • To reflect the fact that it's an isolated effect.

With these in mind, I think it's failed in a number of ways. Take as an example the effect handler for saving a post. On the surface, I really only see two true side-effects:

• The network request
• The notice dismissal
• (And maybe something handling optimist transactions)

With the above "hopes" as a framework, I'd then want (a) each to be its own effect handler, and (b) patterns to emerge for grouping like effects (e.g. all notices, optimistic behaviors).

There is the important consideration of developer experience. Of course, as a developer, it's natural for me to think a save should involve all of the above steps, and in this sense I'd not fault the effect handler for being written the way it is. It should be a critical role of the tools we provide as the framework to guide the developer naturally toward the goals we're hoping to achieve with isolated side effects.

refx, while technically supporting the isolation, fails in promoting it (at least in our current usage). I think colocation is one reason this has occurred:

• The effect handlers are detached from the action creators, even defined in a separate file. This doesn't encourage the developer to consider the side effect as occurring as a result of a particular action. This lack of perceiving a relationship may also contribute to the tendency to group multiple effecting logic together (e.g. notices removal in save effect).
• The effect handlers are not isolated from each other. Instead, we have a single effects.js mega-file (per store). This doesn't encourage the developer to group like behaviors (e.g. notices, optimistic transactions).

So, as all of the above applies to the proposal in this pull request, I've a few observations:

Good:

• There's a benefit to developer experience in enabling developers to consider the effecting action as more closely associated to the original action creator (in this case, part of it).
• As the data API is implemented, we cannot (and don't want to) otherwise support refx-based effects due to its association by action type. Instead, the public interface for working with action dispatching in @wordpress/data is by action creators.
  • Which is to say, this offers a solution which otherwise isn't possible to support in our current refx patterns.

Room for improvement:

• This reverses the dependency by which effects are defined. Where a refx handler binds itself to a specific action type, here the effect is dispatched within the action creator itself.
  • In my mind, defining effects as "attaching" / "hooking" to an initiating action better supports mindset of grouping like behaviors, cascading effects
• We're not really promoting the developer to consider effect isolation since we're not in any way discouraging it. I expect excessively complicated action creators to proliferate.
• With selector side effects, resolvers were implemented as a separate concept from the selector itself. Should we want the same with action creators, or at least seek to reconcile the inconsistency?

To the last point, and in trying to ideate on a "best of all worlds" solution, what if we have side effects defined as associated by the action creator name, in a similar way to selectors:

/* editor/store/actions.js */

function savePost( data ) {
	return { type: 'SAVE_POST', data };
}

savePost.sideEffects = [
	async function requestPostUpdate( action ) {
		const { data } = action;

		try {
			const post = await apiRequest( { data } );
			return savePostSuccess( post );
		} catch ( error ) {
			return savePostError( error );
		}
	},
];

function savePostSuccess( post ) {
	return { type: 'SAVE_POST_SUCCESS', post };
}

savePostSuccess.sideEffects = [
	resetPost,
];

/* editor/store/effects/notices.js */

import { dispatch, registerSideEffects } from '@wordpress/data';

registerSideEffects( 'core/editor', {
	savePost: [
		function dismissNotice() {
			const { removeNotice } = dispatch( 'core/notices' );
			removeNotice( 'SAVE_POST_NOTICE_ID' );
		},
	],
	savePostSuccess: [
		function createSaveSuccessNotice() {
			const { createSuccessNotice } = dispatch( 'core/notices' );
			createSuccessNotice( /* ... , */ { id: 'SAVE_POST_NOTICE_ID' } );
		},
	],
} );

Notes:

• sideEffects must be defined as an array, and used in the plural.
  • Promote idea that it's reasonable / expected to have multiple isolated.
• Side effects can be defined separately via the registerSideEffects API.
  • Grouping like behaviors (notices)
  • Also a pattern for plugins to leverage for hooking into.
• Self-documenting functions.
  • "Unnecessarily" naming functions to clarify their effecting intent. A future maintainer should be disinclined to add unrelated logic.
• No generators yet in this example.
  • I'm not clear yet whether we'd want to support them, but I think it could promote a bad practice of convoluted side effects.
• Statefulness: In writing this (and as already observed in previous comments), we may need state in either action creators or side effects. As implemented in my snippet, it's imposed that the savePost action creator receives a payload. Similarly, we'd need some logic in the requestPostUpdate to determine the endpoint based on the current post type.
  • We don't want to regress on developer experience of using savePost here
  • I should be clear: I'm not strictly opposed to allowing the action creators to be stateful, allowing use of select, making them "unpure". My main concern is isolating behaviors.
  • Maybe we can allow a "before", "replacement" side effect pattern (very much like replaceAction in Framework: Add queried-data state helper utility #6395)

function savePost() {
	return { type: 'SAVE_POST' };
}

savePost.sideEffects = {
	before: [
		function replaceWithPayload( action, selectors ) {
			const { getCurrentPost, getPostEdits, getEditedPostContent } = selectors;
			const post = getCurrentPost();
			const edits = getPostEdits();
			return {
				...action,
				data: {
					...edits,
					content: getEditedPostContent(),
					id: post.id,
				},
			};
		}
	],
	after: [
		async function requestPostUpdate( action ) {
			const { data } = action;

			try {
				const post = await apiRequest( { data } );
				return savePostSuccess( post );
			} catch ( error ) {
				return savePostError( error );
			}
		},
	],
};

And some notes on this:

• Passing selectors subtly discourages a reliance on store, where a large part of our reliance is simply using dispatch directly, when in many cases having access to the selectors is sufficient. Not having access to dispatch forces the developer to create a separate effect handler instead.
• I'm not sure I love "replace" in the context of this being a side effect. It could be a separate pattern on its own (registerActionReplacement, registerActionMiddleware, registerActionAugments).

To reiterate, I'm not trying to promote functional "purity" as much as the goals that these patterns exist to serve (maintenance/scalability benefits of grouping/isolating like/predictable behaviors). If we can introduce statefulness and/or generator yielding to action creators without sacrificing this objective, I'm okay with that. But I'm inclined to think it could promote bad practices (convoluted, unpredictable behaviors).

[youknowriad]

I don’t have the talent to write such long responses :P

I think the proposal is not great in terms of DevX, It’s really hard to understand the flow of things. It’s too difficult to follow what’s happening in these side effects.

We're not really promoting the developer to consider effect isolation since we're not in any way discouraging it. I expect excessively complicated action creators to proliferate.

The improvement I’d make to the generators proposal to solve this is to add composition

function createSaveSuccessNotice( id ) {
   return { type: ‘CREATE_NOTICE’, type: ‘success’, id };
}

function * savePostSuccess( post ) {
    yield { type: 'SAVE_POST_SUCCESS', post };
    yield createSaveSuccessNotice(  'SAVE_POST_NOTICE_ID' );
}

async function * requestPostUpdate( action ) {
	const { data } = action;

	try {
		const post = await apiRequest( { data } );
		yield savePostSuccess( post );
	} catch ( error ) {
		yield savePostError( error );
	}
}

We can even support multiple effects at the same time

async function * requestPostUpdate( action ) {
	yield parallel( [

		effect1,
		effect2
	] );
}

I think this “imperative-like” flow for side effects is better than indirection. It makes things easier to reason about.

Whether these side-effects should be declared separately from the action creator, I don’t think it’s a good idea for two reasons:

• We can compose side-effects and actions. (Actions are just effects changing the state)
• DevX: I feel it’s an unnecessary complication.

Regarding access to selectors and actions from other stores. I think we should just call. select/dispatch but there’s room for improvements here.

[aduth]

The improvement [...] is to add composition

But as has been demonstrated very clearly in effects.js, developers won't do it. Without a deterrent or obvious advantage to an alternative, we'll bundle it all in the requestPostUpdate implementation.

It makes things easier to reason about.

On the singular perspective of: What happens when we call requestPostUpdate. And I'd agree, at least so far as not needing to leave the function to follow the indirection, and having full awareness of what all occurs as a result of that action.

But what about:

• How do I reason about when notices are added or removed anywhere in the system?
• If Introduce a dedicated autosaves endpoint for handling autosave behavior #6257 is merged as proposed, the full implementation of the post update is 77 lines. At what point do we start losing the ability to reason about it easily, merely for the fact that these are not considered as individual tasks of the action?
• Do we want to support plugins hooking into the dispatch of an action? How would we do this? I've always thought refx-like effects handlers would be a fine enough solution, with the main hesitation being a reluctance to expose the internal action types. With the implementation here, we're moving away from this being an option; at least one which is also first-class in the sense it's used in the core implementations.

[youknowriad]

How do I reason about when notices are added or removed anywhere in the system?

I don't understand the question, you call await dispatch( 'core/notices' ).createSaveSuccessNotice( id' ); when you want to dispatch a notice. In the example above I wanted to show how we could compose actions from the same store. Or do you think it's not obvious enough?

If #6257 is merged as proposed, the full implementation of the post update is 77 lines. At what point do we start losing the ability to reason about it easily, merely for the fact that these are not considered as individual tasks of the action?

I think the difference is that I'm fine with long action creators or long effects :). I'd isolate effects only when needed in two places. I don't really care about separating them just because it's conceptually separate effects.

Do we want to support plugins hooking into the dispatch of an action?

I don't know 🤷‍♂️ This PR obviously doesn't address this but is it really a problem? I don't know the answer, I didn't felt the need for this personally. Wouldn't a simple wp.hooks.doFilter( action )/wp.hooks.doAction( action ) be enough for this?

---

I don't really care too much about the API we end up using, just sharing my thoughts. Actually, I'd prefer if we bake in refx instead because it's better in DevX :) But as I said, I think that's because I don't care about separation of effects into smaller effects unless it's necessary.

[aduth]

I don't understand the question, you call await dispatch( 'core/notices' ).createSaveSuccessNotice( id' ); when you want to dispatch a notice. In the example above I wanted to show how we could compose actions from the same store. Or do you think it's not obvious enough?

The question was not so much "How do I create or remove notices", it's a question of: As someone merely reading the code, how can I see where the code notices currently occur?

Put another way, let me pose the question: What notices do we have in Gutenberg right now? Once you've come up with an answer, think about the process you undertook to come to that conclusion. Is it better or worse to have notices behavior grouped together separately as a single set of related effects, or to closely associate them individually with e.g. saving, deleting, trashing (and every other future case where we need them)? I don't know the answer; just noting that perception of reasoning about is more complicated than just what occurs in a single action, but also making sense of the system as a whole.

I think the difference is that I'm fine with long action creators or long effects :)

And in just the same way, we could have the entire implementation of Gutenberg occur within initializeEditor 😄 As with anything, it's a question of optimizing the balance between (a) having everything in one place, (b) being able to understand, at a glance, what is happening, and (c) having the confidence and ability to make changes safely.

I don't know 🤷‍♂️ This PR obviously doesn't address this but is it really a problem?

It's a problem in that we have the same need in the core implementation as is needed by plugins. But I may feel more strongly about this because I'm thinking of things like notices as incidental to the act of saving itself, as a separate behavior that occurs as a mere consequence of the action, in much the same way a plugin would want to hook in.

[aduth]

As I look to resolve #7066, my first hunch was to move the setting of the URL from editor (where the save action dispatching occurs) to edit-post, which reminded me that we have a bunch of these not-so-ideal change detections currently to achieve the same sort of goal. In this case, I need to observe when the post ID (or more accurately, when the parent post ID) changes, and update the URL accordingly. I could implement this by adding another onChangeListener, but with an integrated side effects solution like in #6999 (comment), this could also look something like:

// edit-post/store/tbd.js

registerSideEffects( 'core/editor', {
	resetPost: replaceURLForPost
} );

[youknowriad]

Thinking more about this, I think we're trying to solve two things that are different.

• Side effects: Like the examples, you're showing (notices, Url ...)
• Async actions: save post, update config where a side effect solution forces us to create unwanted action types just to trigger the side effect. You can argue that these action types are not unwanted and are useful to do optimistic updates but I don't think that's true because if you don't need to make an async action, you don't need optimitic updates/reverts which means the behavior of the action is different if it's part of an async action or if it was a sync one.

[youknowriad]

superseded by #8096