Incorporate refx as Redux side effect pattern

[aduth]

Closes #691

This pull request seeks to implement a pattern for managing side effects, in many cases but not exclusively network requests incidentally incurred by dispatched action intents. Doing so eliminates our pattern for passing dispatch as an argument to pseudo-action creators, standardizing on a single synchronous return value for action creators defined in actions.js, moving side-effects into a separate effects.js file. This implementation is achieved using refx, a middleware for "watching" actions.

See also #691 for related discussion.

Implementation notes:

Behaviors of merging, trashing, and saving posts should be unaffected by these changes, with one exception: When a post save completes, it appeared to me we should be updating the post's state isNew value to false. @nylen let me know if this was intended to remain true.

Testing instructions:

Verify that there are no regressions in saving, trashing, or merging blocks:

• Save post, noting that post is saved and URL updates with post ID
• Trash post, noting that post is trashed and you are redirected to posts list
• Merge two blocks, for example the first two text blocks

Ensure that tests pass:

npm test

[nylen]

I'll give this a more detailed review tomorrow, but +1 on the approach, as discussed at #691.

When a post save completes, it appeared to me we should be updating the post's state isNew value to false. @nylen let me know if this was intended to remain true.

I think the value before the changes here is correct; however, this state key is a bit confusing and probably needs to be restructured or at least named better. This is not the main isNew property; instead, this is used to determine the message on the publish button during and after a post is being saved. If I were expressing the value of this property in English I would say "is the current request for a new post OR was the previous request for a new post".

See the isSavingNewPost selector and the requestIsNewPost property of the PublishButton for its usage.

[BE-Webdesign]

Will checkout tomorrow, if that is not quick enough skip over my review 😄

[youknowriad]

How do you decide to extract actions like this as a separate effect (since it's not used explicitly from the App)?

[youknowriad]

Just a crazy idea: What if we write the effects as thunks to make them "compatible" with the way the thunk middleware works? REQUEST_POST_UPDATE = ( action ) => ( dispatch ) => {}

[aduth]

Just a crazy idea: What if we write the effects as thunks to make them "compatible" with the way the thunk middleware works? REQUEST_POST_UPDATE = ( action ) => ( dispatch ) => {}

Funny you mention it, a similar idea had been on my mind yesterday for a reason I don't recall. In my case thinking to align with the middleware signature ( store ) => ( action ) => ( next ) to allow these to act as mini-middlewares of sorts. In the end I didn't find it particularly compelling outside the consistency point, and in fact I've been trying to push refx more in a direction of encouraging synchronous flows, discouraging the store argument as much as possible (not always possible).

See also:

• v2.0.0 Changelog notes
• Example in practice, using a separate requests middleware

[youknowriad]

LGTM 👍 Thanks for doing this

[BE-Webdesign]

Minor: blocks appears to only be used once in this function. Is there an advantage to destructing then assigning? This could be condensed to [ blockA, blockB ] = action.blocks with one less line.

[aduth]

Minor: blocks appears to only be used once in this function. Is there an advantage to destructing then assigning? This could be condensed to [ blockA, blockB ] = action.blocks with one less line.

Sure, that seems reasonable.

[BE-Webdesign]

Minor: I think it would be nicer to extract both, REQUEST_POST_UPDATE_SUCCESS and REQUEST_POST_UPDATE_FAILURE into an action creator, obviously not a high priority and maybe can be done if/when these actions are triggered elsewhere, to ensure consistency.

[aduth]

Minor: I think it would be nicer to extract both, REQUEST_POST_UPDATE_SUCCESS and REQUEST_POST_UPDATE_FAILURE into an action creator, obviously not a high priority and maybe can be done if/when these actions are triggered elsewhere, to ensure consistency.

I've been thinking on success and failure types generally, and would like to revisit them in a subsequent pull request. Historically the triple of REQUEST, _SUCCESS, and _FAILURE types has been a pain point for me and I don't think they're always necessary. I've talked with a few people about what it means to eliminate them in favor of (a) more optimistic state changes and (b) more semantic "received updated version" actions. For example, something like this should ideally not be tied so strongly to the success case, but rather the success case dispatching an action indicating that the latest version of the post has been received. Where success and failure types could be necessary is in indicating progress, such as the "Saving..." text to be shown on the publish button, or in presenting errors in the UI.

[BE-Webdesign]

I think I am following you, please correct me if I am missing the mark (I often do 😅 ) You are concerned over the coupling of the request success action to how it is being used in the reducer? In my apps I usually have a separate action like INSERT_{ENTITY}_INTO_{STORE} or when deleting REMOVE_{ENTITY}_FROM_{STORE}. That can serve multipurpose cases rather than just network. These are often triggered by the success/failure network actions. They work well to solve what I think you are bringing up as a problem. I might not understand though, so hopefully that helps. Would love to hear how you handle this as well, or plan to.

[aduth]

That can serve multipurpose cases rather than just network. These are often triggered by the success/failure network actions.

Yep, this is exactly the sort of thing I'd hope for.

[BE-Webdesign]

👍 Tests passed and I think everything works as expected. Hooray for refx!

[nylen]

@aduth the isNew changes here introduced some minor breakage. When saving a new post, the publish button should say "Saving..." and then "Saved!" Instead it now says "Updating..." and then "Updated!" Explanation and links to code above at #907 (comment).

[aduth]

Oh dang, I had meant to revisit that but clearly forgot. I'll spin up a fix pull request.

[nylen]

It's nice to see that we can easily test actions now (#920). However, it's not so clear to me how we can add tests for the effects callbacks such as REQUEST_POST_UPDATE. I suppose we still need a way of mocking out the API call. Any thoughts there?

[BE-Webdesign]

I suppose we still need a way of mocking out the API call. Any thoughts there?

Yup we will need to mock the request. I use Nock. I am going to have some free time coming up next week and was planning on allocating that to helping out on Gutenberg Documentation/Unit Testing, so I can tackle this on Monday.

I haven't used refx in any of my projects yet, but I imagine we will import the effects and then call each of the effect's functions individually and use a mock store as well. Simply verifying that the sequence of actions that trigger from the async call match our expectations, is also enough coverage in my opinion, so we don't necessarily need to test the effects but test the mock store, triggering the effects. Mocking is not ideal, but it becomes somewhat necessary for integration tests like these. I have not found a better solution yet and would love to have some aduth or riad knowledge dropping to show me the way.

[nylen]

Gutenberg Documentation/Unit Testing

❤️ ❤️ ❤️ ❤️ ❤️

Feel free to ping me for review on anything you put together.

[youknowriad]

@BE-Webdesign Mocking and unit-testing is one of the reasons I like the generators approach so much. Because when using generators the processing is split between two things:

1- the generator itself which doesn't trigger any side-effect (I like to call them "pure-generators") but instead trigger "effects" (which are just plain javascript object)

2- the generator runtime (rungen, or redux-saga) which is responsible of handling those effects and effectively running them. So in unit tests, you don't need to run these effects, you just need to provide the result values without mocking anything.

For example, you could have a this:

// a function to isolate the API call
function savePostApi( postId, edits ) {
   return superagent.post( '/url/' + postId, edits );
}

// The generator action creator
function* savePostAction( postId, edits ) {
  const savedPost = yield { effect: 'call', func: savePostApi, args: [ postId, edits ] };
  yield { effect: 'dispatch', action: { type: 'POST_SAVED_SUCCESS', postId, savedPost } };
}

You'll notice the generator doesn't call the effects directly but just describes what needs to be done. And automatically the generator runtime (which is the redux middleware in our case) could do different things depending on the effect

Thus, unit testing this is as simple as playing the role of the runtime by providing the result of yield without even running or mocking any side effect. Something like this

const postId = 1;
const edits = { // some random edits };
const savedPost = { // some random saved post object };
const myGen = savePostAction( postId, edits );

expect(myGen.next().value).toEqual( { effect: 'call', func: savePostApi, args: [ postId, edits ] } );
expect(myGen.next( savedPost ).value).toEqual( { effect: 'dispatch', action: { type: 'POST_SAVED_SUCCESS', postId, savedPost } } ); 

And that's it. (You'll notice I'm very passionate about generators :P I wrote several blog posts about this stuff. https://riad.blog/2015/12/28/redux-nowadays-from-actions-creators-to-sagas/)

---

That said, with refx, I think mocking is required because the code of the action creators is imperative and not declarative.

[BE-Webdesign]

Yeah, that is definitely nicely testable, I tinkered with generators a little bit, maybe I will and should tinker more with them. I will check out your library and blog posts! I like how the generators also appear to be entirely separate from the store as well. When do you feel generators are at their peak of usefulness?

How would you go about testing savePostApi() though? I imagine mocking would still be needed then? I can see how generators would be very good for control flow. To isolate our networking, I was thinking about turning the effects into something more like this:

const savePostRequestEffect = ( networkCallback ) => ( action, store ) {
    // Do my stuff.
    // At some point call our network callback and apply any args needed.
   networkCallback( args )
   // Resolve stuff, dispatch other actions.
}

// Replace fetch() below with the wp-api stuff.
const savePostApi = ( args ) => fetch( args )

const effects = {
    // Partially apply the callback to be called back inside.
    REQUEST_SAVE_POST: savePostRequestEffect( savePostApi )
}

By using partial application we can easily send in a mock or different testable callback to our effects. It is not quite as declarative as your generators example, but is well testable in my opinion.

[aduth]

I think mocking is required because the code of the action creators is imperative and not declarative.

In what way are they imperative?

[nylen]

I mean to say effects here. "Send an API request" rather than "May an API request be sent."

[youknowriad]

This is an imperative code.

fetch( args ).then( posts  => { // do something } )

We're imperatively triggering the side effect inside the action creator.

While writing this

const posts = yield call( fetch, args )

This code is not imperative because this code doesn't trigger any side effect explicitely, we're just declaring that "to produce posts, you need to trigger this effect, but we're not imperatively triggering the call in the generator itself.

I hope this explains my point.

[aduth]

At some point the fetch will need to occur. We could isolate this with generators, yes, and we can also do so with effects. From the posts fetching effect handler, we could return a { type: 'FETCH' } and create a separate handler for generically handling fetches.

How this interacts with WP-API.js is another question though.

[youknowriad]

we could return a { type: 'FETCH' } and create a separate handler for generically handling fetches.

Why not? this object is what I call "effect", the function itself is just an action creator or an effect creator for me.

[nylen]

At some point the fetch will need to occur.

Note I'm not trying to say that the current structure is bad. Side-effects have to be imperative at some point, I just wanted to comment so that we think about how/whether to test these remaining bits of tricky code.

WP-API.js has unit tests in core which provide mocking: https://core.trac.wordpress.org/browser/tags/4.7.5/tests/qunit/fixtures/wp-api.js

[aduth]

Why not?

Might be some misunderstanding. I'm claiming the refx effect handler can return an action of this form.

[youknowriad]

Might be some misunderstanding.

Actually, I understood correctly, but I don't see a way to use the "effect" result using refx.
The main idea here is to isolate granular effects, so we can test the logic easily.

[BE-Webdesign]

I just wanted to comment so that we think about how/whether to test these remaining bits of tricky code.

I've got the tests coming and they are working well. Give me a a bit.

[dmsnell]

I'll chime in and say that I think the way it is as-is is fair for this project. If we were going to produce any further separation of concerns I would recommend following what I did in Calypso's data layer with HTTP requests and make the split and the "language of the system" level. Saving a post is a concern of the editor language. Making a GET request is a concern of the network language.

The reason I have rejected promises in middleware handlers in Calypso is a direct result of this. A promise or generator are ultimately coupled in the session itself but need not be. If we, for example, were to perform offline queuing and error resolution then we need to be able to decouple an original success/failure responder from the outbound request. This can happen with a description of what should happen on response instead of actually just doing stuff on response.

The question I ask is whether there will be enough varied network activity in this project to warrant that much of a system or if it is small enough that doing what @aduth has presented is enough separation to meet our needs and wants.

2¢, I'm happy to discuss further

[aduth]

but I don't see a way to use the "effect" result using refx

If an effect handler returns an object, it's dispatched as an action. This gives you some flexibility in chaining effects.