Use redux-thunk for side effects #176

kumar303 · 2019-02-08T17:33:15Z

Fixes #12

After some consideration (see below), redux-thunk is a pretty good option. This adds redux-thunk for handling side effects.

Original PR description:

As part of #12, here's an example of using redux-thunk so we can assess it.

Here are some questions worth asking:

How testable is this?
Does it allow our application behavior to be traced through one place?
[maybe more questions]

Known issues:

The (dispatch as ThunkDispatch) type hint seems like it should be unnecessary since we have types for our middleware (see Use with TypeScript 2.0 (@types/redux-thunk) reduxjs/redux-thunk#103) but I couldn't get the error to go away without this type hint, even when doing the same module override.
I don't like how the execution of requestLogOut() is not captured in action replay. When we used sagas, it was possible to replay the stack of actions and re-execute each saga. This reduces traceability. As an example, let's say we were given a stack of actions by someone who encountered an error thrown in a thunk. The stack of actions would not let us reproduce that error.

kumar303 · 2019-02-08T17:33:57Z

@willdurand @bobsilverberg let me know what you think of thunks! I added questions and known issues to the PR description.

codecov-io · 2019-02-08T17:35:07Z

Codecov Report

Merging #176 into master will decrease coverage by 0.67%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   94.77%   94.09%   -0.68%     
==========================================
  Files          20       20              
  Lines         287      305      +18     
  Branches       71       74       +3     
==========================================
+ Hits          272      287      +15     
- Misses         14       17       +3     
  Partials        1        1

Impacted Files	Coverage Δ
src/components/Navbar/index.tsx	`100% <100%> (ø)`	⬆️
src/test-helpers.tsx	`100% <100%> (ø)`	⬆️
src/reducers/users.tsx	`88.88% <60%> (-11.12%)`	⬇️
src/configureStore.tsx	`69.23% <66.66%> (+6.73%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfd753e...6293dcc. Read the comment docs.

willdurand · 2019-02-08T17:37:53Z

* The `(dispatch as ThunkDispatch)` type hint seems like it should be unnecessary since we have types for our middleware (see [reduxjs/redux-thunk#103](https://github.com/reduxjs/redux-thunk/issues/103)) but I couldn't get the error to go away without this type hint, even when doing the same module override.

It is a dumb question but: isn't it that ConnectedReduxProps declares dispatch as Dispatch and not ThunkDispatch?

willdurand · 2019-02-08T17:41:24Z

I don't like how the execution of requestLogOut() is not captured in action replay. When we used sagas, it was possible to replay the stack of actions and re-execute each saga. This reduces traceability. As an example, let's say we were given a stack of actions by someone who encountered an error thrown in a thunk. The stack of actions would not let us reproduce that error.

This could be done by dispatching an action at the beginning of the thunk as shown here: #12 (comment). AFAIK you did not like that an action intercepted by redux-saga was automatically replayed either. I think we have the best of both worlds if we use thunk + the pattern that consists in dispatching an action early in the thunk.

kumar303 · 2019-02-08T17:43:41Z

This could be done by dispatching an action at the beginning of the thunk

How would that solve it? When replying actions, you would just be replaying the first action, not re-executing the entire thunk function.

kumar303 · 2019-02-08T17:45:37Z

isn't it that ConnectedReduxProps declares dispatch as Dispatch and not ThunkDispatch?

Yes, that is correct and that is the problem. A thunk is a function, not an action, so if you edit the code in this patch to do dispatch(_requestLogOut()) then you get an error saying that _requestLogOut() doesn't appear to be an action (indeed it's not).

willdurand · 2019-02-08T17:47:00Z

How would that solve it? When replying actions, you would just be replaying the first action, not re-executing the entire thunk function.

I am not sure to follow. You want the execution of the thunk to be captured. What do you expect precisely? I read it as "I want to know that the thunk has been executed". Which other information is missing otherwise?

willdurand · 2019-02-08T17:47:54Z

A thunk is a function, not an action

The middleware changes the behavior so that you can dispatch both, so why do reflecting that in the type? dispatch: Action | Thunk?

willdurand · 2019-02-08T17:57:33Z

src/components/Navbar/index.spec.tsx

+        creator: jest.fn().mockReturnValue(dispatchCallback),
+        dispatchCallback,
+      };
+    };


I believe this would be easier to follow if we had different names:

const dispatchCallback = '__dispatchCallbackReturnedByTheThunk__'; const _requestLogOut = jest.fn().mockReturnValue(dispatchCallback); expect(dispatch).toHaveBeenCalledWith(dispatchCallback);

I don't really understand the API of the object returned by createFakeThunk.

I'm not sure what to add except that I too find this test confusing. Maybe there's a way to make it clearer. I think @willdurand's suggestions would help.

willdurand · 2019-02-08T18:00:59Z

src/reducers/users.spec.tsx

+        dispatch,
+        thunk: () => thunk(dispatch, () => store.getState(), undefined),
+        store,
+      };


I like this helper function!

willdurand · 2019-02-08T18:01:44Z

src/test-helpers.spec.tsx

@@ -11,6 +11,7 @@ describe(__filename, () => {
    };

    const wrapper = () => {
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any


I guess this resolves #174 :D

kumar303 · 2019-02-08T18:25:27Z

You want the execution of the thunk to be captured. What do you expect precisely?

I expect to preserve this key principle of Redux as much as possible:

All of the behavior of your application can be traced through one place, and that behavior can be easily broken apart and composed back together.

When not relying on side effects, this principle can be achieved by simply dispatching actions:

dispatch LOG_IN -> all component code to render a logged in state is executed
dispatch LOAD_USER -> all component code to render user data is executed

We can replay these two actions and execute the exact code that led us to these UI states. We cannot replay the click handler functions which dispatched each action but as long as those handler functions do nothing but dispatch actions then the risk of breakage there is minimal.

Let's say we needed to rely on side effects and we implemented them with sagas. Dispatching actions would look more like this:

dispatch REQUEST_LOG_OUT ->
- code in saga: make an API request to log out
- code in saga: dispatch LOG_OUT or dispatch ERROR

We can now replay the action REQUEST_LOG_OUT and execute the exact code that led us to all UI states, including the code that interacted with the API. This is appealing to me because it would theoretically let us reproduce a bug inside a saga given just a stack of actions. It also helps one to understand how the app works because you can always trace behavior to an action. The risk of breakage within side effect handlers (such as a saga) is not minimal. There are lots of things that could break so it's important to have traceability here.

Here's how it would look with a thunk:

user clicks logout button, executes thunk
- code in thunk: make an API request to log out
- code in thunk: dispatch LOG_OUT or dispatch ERROR

We can replay the LOG_OUT or ERROR actions but we cannot replay the API request when given a stack of actions.

I'm going to try redux-loop next to see if it lets us achieve traceability.

willdurand · 2019-02-08T19:25:56Z

Thanks for the detailed reply, now I get it.

We can replay the LOG_OUT or ERROR actions but we cannot replay the API request when given a stack of actions.

As per #12 (comment), this is exactly what you wanted to avoid though:

As soon as you time travel to FETCH_USER, it re-runs the saga, makes the API request again, and dispatches more actions.

That being said, I understand that replaying a thunk is difficult but given a state and a stack of actions, you will still be able to reproduce what has happened, without firing the API call. I mean, in terms of data flow, you would get the same end state.

Unless there is an issue with the fetch implementation that is not caught in a try/catch, I think replaying the HTTP call is actually not needed. Redux actions describe the "results" of side-effects, so replaying these actions should be enough to retrieve the same state, even if those side-effects did not fire again.

The redux devtools allow to load a state and to replay a stack of actions, so if we can record the stack of actions, we could also retain the initial state and either go forward or backward (if we can capture the end state too for example)

kumar303 · 2019-02-08T19:49:48Z

As per #12 (comment), [triggering side effects in the developer tool] is exactly what you wanted to avoid though

Yes, that's true. It's because I mostly want to use the developer tool to develop screens and sometimes I like to hop around between states. We had the habit of rendering important screens using the same action that triggered a saga and that made it hard to time travel. You could do it but the stack of actions would keep growing and growing which made it hard to move forward after moving backward.

You raised a good point in that conversation about how you do want to see side effects in the developer tool. I get that, too. Ideally, I'd like a checkbox to temporarily disable side effects.

...replaying a thunk is difficult but given a state and a stack of actions, you will still be able to reproduce what has happened, without firing the API call. I mean, in terms of data flow, you would get the same end state.

Yes, but all of the code executed in between is very crucial to the program flow, especially in trying to track down unexpected errors. The same would go for rendering a component: when replaying an action I would very much want to see an error thrown by render(), for example. Otherwise, what's the point of using Redux?

I would like to store a stack of actions as Sentry breadcrumbs one day 💡 In dev / stage only, of course! 😎

bobsilverberg

This looks pretty good to me, thanks @kumar303. It feels a bit weird to have the thunk code right in the reducer - I guess I'm used to having the sagas in their own files - but I think it's okay.

I would be fine with us taking this approach, but if you're also going to do an example with redux-loop I'd love to see that as well.

bobsilverberg · 2019-02-11T16:14:12Z

src/components/Navbar/index.spec.tsx

+        creator: jest.fn().mockReturnValue(dispatchCallback),
+        dispatchCallback,
+      };
+    };


I'm not sure what to add except that I too find this test confusing. Maybe there's a way to make it clearer. I think @willdurand's suggestions would help.

bobsilverberg · 2019-02-12T13:49:16Z

src/components/Navbar/index.tsx

-    dispatch(userActions.logOut());
+  logOut = () => {
+    const { _requestLogOut, dispatch } = this.props;
+    (dispatch as ThunkDispatch)(_requestLogOut());


I saw your comment about this and yes, it would be better if we could find a way to not have to do this type hinting. It seems like the component dispatching the action shouldn't even need to know if it's a "plain" action or a thunk action.

src/reducers/users.spec.tsx

kumar303 · 2019-02-20T05:29:01Z

It is a dumb question but: isn't it that ConnectedReduxProps declares dispatch as Dispatch and not ThunkDispatch?

Not dumb at all! The declaration of ConnectedReduxProps just needed an update -- that was the problem and I somehow misread your comment when I first replied to it. Thanks. 😓

kumar303 · 2019-02-20T06:05:24Z

@bobsilverberg @willdurand I think we should go with thunks. I still dislike how thunks are not traceable given actions alone but since they don't require generator functions, they are probably our best option, IMO. Could you take another look at the patch? I have abandoned my attempt at getting redux-loop to work (#229).

willdurand

I like this patch, especially the testing part, thanks! That works for me.

willdurand · 2019-02-20T15:16:54Z

src/test-helpers.tsx

+ *
+ * You can make an assertion that it was called like:
+ *
+ * expect(dispatch).toHaveBeenCalledWith(fakeThunk.thunk);


kumar303 · 2019-02-20T16:11:28Z

Thanks for the review. @bobsilverberg I'm going to merge this to keep it going but let me know if it needs any follow-ups from your point of view. We can always revisit this side effect strategy as we begin using it more.

kumar303 requested review from bobsilverberg and willdurand and removed request for bobsilverberg February 8, 2019 17:33

willdurand reviewed Feb 8, 2019

View reviewed changes

bobsilverberg reviewed Feb 12, 2019

View reviewed changes

kumar303 added 6 commits February 19, 2019 16:03

Try out redux-thunk

b8ab3ce

Add ThunkActionCreator

fdb9c24

Fix ThunkMiddleware

772aa66

Add alias for ThunkDispatch

a591bad

Lint cleanup

cc84fea

Prettier

c41bea7

kumar303 force-pushed the try-redux-thunk branch from 04f4e64 to c41bea7 Compare February 19, 2019 22:12

kumar303 mentioned this pull request Feb 19, 2019

Add better return types for the API functions #218

Merged

kumar303 added 3 commits February 19, 2019 22:53

Maybe some better comments

d665ec9

Moved helper to test-helpers

364efbe

Fixed type of dispatch in ConnectedReduxProps. Duh.

8dc2f62

Better names for things, maybe

52fa64a

kumar303 added 2 commits February 19, 2019 23:54

A better thunk tester

9f40d78

Simplified use of this helper

95beacd

kumar303 changed the title ~~[WIP] Example using redux-thunk~~ Use redux-thunk Feb 20, 2019

kumar303 requested review from bobsilverberg and willdurand February 20, 2019 06:02

Fix lint

6293dcc

willdurand approved these changes Feb 20, 2019

View reviewed changes

kumar303 changed the title ~~Use redux-thunk~~ Use redux-thunk for side effects Feb 20, 2019

kumar303 merged commit 1c9bd54 into mozilla:master Feb 20, 2019

kumar303 deleted the try-redux-thunk branch February 20, 2019 16:12

Use redux-thunk for side effects #176

Use redux-thunk for side effects #176

Uh oh!

Conversation

kumar303 commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 commented Feb 8, 2019

Uh oh!

codecov-io commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

willdurand commented Feb 8, 2019

Uh oh!

willdurand commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 commented Feb 8, 2019

Uh oh!

kumar303 commented Feb 8, 2019

Uh oh!

willdurand commented Feb 8, 2019

Uh oh!

willdurand commented Feb 8, 2019

Uh oh!

willdurand Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

bobsilverberg Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

willdurand Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

willdurand Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

kumar303 commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willdurand commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobsilverberg left a comment

Choose a reason for hiding this comment

Uh oh!

bobsilverberg Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

bobsilverberg Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kumar303 commented Feb 20, 2019

Uh oh!

kumar303 commented Feb 20, 2019

Uh oh!

willdurand left a comment

Choose a reason for hiding this comment

Uh oh!

willdurand Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

kumar303 commented Feb 20, 2019

Uh oh!

Uh oh!

kumar303 commented Feb 8, 2019 •

edited

Loading

codecov-io commented Feb 8, 2019 •

edited

Loading

willdurand commented Feb 8, 2019 •

edited

Loading

kumar303 commented Feb 8, 2019 •

edited

Loading

willdurand commented Feb 8, 2019 •

edited

Loading

kumar303 commented Feb 8, 2019 •

edited

Loading