1215251 - Create a Task middleware for redux

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Would allow dispatching generator functions. These generator functions would be called with two params: dispatch and getState, just like the thunk middleware.

The middleware would keep pumping the generator and whenever it yielded or returned a promise, wait on the promise and then keep pumping. Just like Task.jsm. The final return value (after being resolved if it is a promise) would be another action to be dispatched.

Example usage:

function takeSnapshotAndCensus(front, worker) {
  return function* (dispatch, getState) {
    dispatch(savingHeapSnapshot());
    const snapshotId = yield front.saveHeapSnapshot();

    dispatch(readingHeapSnapshot());
    yield worker.readHeapSnapshot(snapshotId);

    dispatch(takingCensus());
    const report = yield worker.takeCensus(snapshotId,
                                           getState().breakdown);

    dispatch(showCensusResults(report));
  };
}

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1215251-defer-middleware.patch

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8674485 [details] [diff] [review]
1215251-defer-middleware.patch

Review of attachment 8674485 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/redux/middleware/defer.js
@@ +27,5 @@
> +function defer ({ dispatch, getState }) {
> +  return next => action => {
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {
> +        return yield action(dispatch, getState); 

fixed white space

::: devtools/client/shared/redux/middleware/test/test_middleware-defer-02.js
@@ +39,5 @@
> +  return { type: "fetchSync", data };
> +}
> +
> +function fetchAsync (data) {
> +  return function *(dispatch) { 

fixed white space

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=500719a1a0b0

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8674485 [details] [diff] [review]
1215251-defer-middleware.patch

Review of attachment 8674485 [details] [diff] [review]:
-----------------------------------------------------------------

I really think this middleware should be named "task" and not "defer".

We need tests for:

* Rejected promises
* Errors thrown in tasks
* Many yields in a task, not just one

Also, a couple questions I'd like to make sure I understand correctly.

Will re-review super quick when you get the patch back to me.

::: devtools/client/shared/redux/middleware/defer.js
@@ +11,5 @@
> +}
> +
> +function isPromise (action) {
> +  return action && typeof action.then === "function";
> +}

These helpers seem like they could go in DTU if you're so inclined.

@@ +27,5 @@
> +function defer ({ dispatch, getState }) {
> +  return next => action => {
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {
> +        return yield action(dispatch, getState); 

I think this should be:

  return yield* action(dispatch, getState);

(Note the "*")

Alternatively, don't make your own generator and instead bind the action itself:

  return Task.spawn(action.bind(null, dispatch, getState));

@@ +29,5 @@
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {
> +        return yield action(dispatch, getState); 
> +      }).then(value => {
> +        if (value) {

Is 1 a valid action? What about zero? Do actions have to be objects/functions/generators/promises? Should this be an explicit undefined check rather than truthiness check?

@@ +33,5 @@
> +        if (value) {
> +          return dispatch(value);
> +        }
> +      });
> +    }

Nit: can we add some blank lines between each branch? Feeling a little claustrophobic as I try and understand everything here. Like:

  ...
}
<empty line>
if (isPrimise(action)) {
  ...

@@ +36,5 @@
> +      });
> +    }
> +    if (isPromise(action)) {
> +      return action.then(dispatch, err =>
> +        executeSoon(() => reportException("@@redux/middleware/defer", err)));

Ok, and the reason we don't add the error handler after spawning the task is that it will be added here?

::: devtools/client/shared/redux/middleware/test/test_middleware-defer-01.js
@@ +32,5 @@
> +
> +function fetch1 (data) {
> +  return function *(dispatch, getState) {
> +    equal(getState().length, 0, "`getState` is accessible in a generator action");
> +    let moreData = yield new Promise(resolve => resolve(data));

Can you yield twice here? I suspect this may uncover the missing "*" bug I mention above.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8674485 [details] [diff] [review]
1215251-defer-middleware.patch

Review of attachment 8674485 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/redux/middleware/defer.js
@@ +11,5 @@
> +}
> +
> +function isPromise (action) {
> +  return action && typeof action.then === "function";
> +}

Good idea -- isPromise is especially used a lot, since instanceof won't work for our garbage bag full of implementations

@@ +27,5 @@
> +function defer ({ dispatch, getState }) {
> +  return next => action => {
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {
> +        return yield action(dispatch, getState); 

Reason this spawns a generator is so it doesn't lose any `bind` used, but probably an edge case; I don't feel strongly about it.

+1 on yield*

@@ +29,5 @@
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {
> +        return yield action(dispatch, getState); 
> +      }).then(value => {
> +        if (value) {

No -- only objects are valid, but this mimics the promise implementation of dispatching the resolved value -- Redux will complain immediately if attempting anything other than an object

@@ +36,5 @@
> +      });
> +    }
> +    if (isPromise(action)) {
> +      return action.then(dispatch, err =>
> +        executeSoon(() => reportException("@@redux/middleware/defer", err)));

I'm not quite sure what this means, but this lets us throw outside of a promise

::: devtools/client/shared/redux/middleware/test/test_middleware-defer-01.js
@@ +32,5 @@
> +
> +function fetch1 (data) {
> +  return function *(dispatch, getState) {
> +    equal(getState().length, 0, "`getState` is accessible in a generator action");
> +    let moreData = yield new Promise(resolve => resolve(data));

Probably, but will add to make sure

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Adding more test cases. Is `task` appropriate even if it handles promises too? Was trying to think of something to describe both, but I guess it's still promises and generators under the hood anyway.

Comment 7

[James Long (:jlongster)]

Comment on attachment 8674485 [details] [diff] [review]
1215251-defer-middleware.patch

Review of attachment 8674485 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/redux/middleware/defer.js
@@ +26,5 @@
> + */
> +function defer ({ dispatch, getState }) {
> +  return next => action => {
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {

I really think this just needs to be:  `return Task.spawn(action.bind(null, dispatch, getState));` 

See my other comment in your test.

::: devtools/client/shared/redux/middleware/test/test_middleware-defer-02.js
@@ +40,5 @@
> +}
> +
> +function fetchAsync (data) {
> +  return function *(dispatch) { 
> +    return new Promise(resolve => resolve({ type: "fetchAsync", data }));

If you want to dispatch this action, this should be:

`return new Promise(resolve => resolve(dispatch({ type: "fetchAsync", data })));`

That action will still be resolved, but now it's also dispatched. This works with my 1-line version of the middleware in my other comment. You shouldn't be dispatching it in the middleware imho.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8674485 [details] [diff] [review]
1215251-defer-middleware.patch

Review of attachment 8674485 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/redux/middleware/defer.js
@@ +26,5 @@
> + */
> +function defer ({ dispatch, getState }) {
> +  return next => action => {
> +    if (isGenerator(action)) {
> +      return Task.spawn(function*() {

Changed this in lieu of previous comments

::: devtools/client/shared/redux/middleware/test/test_middleware-defer-02.js
@@ +40,5 @@
> +}
> +
> +function fetchAsync (data) {
> +  return function *(dispatch) { 
> +    return new Promise(resolve => resolve({ type: "fetchAsync", data }));

This is returning a promise of an action -- redundant actually since this is wrapped in task, but an extra check.

Without handling the return values, this line fails:

`data.async = yield dispatch(fetchAsync("async"));`

And that's pretty important IMO, as then the dispatch function still returns an action (as these are action creators) so they can be consumed and composed by other tasks. How would you use this independently and in another task?

```
function fetchUser (user, key) {
  return function *(dispatch) {
    let data = yield XHR(user, key);
    return { type: "fetched-user", data };
  }
}
```

It could function by itself if we handled the dispatch there instead of dispatching the return implicitly, but then consuming it would be:

```
function complicatedTask (user, creds) {
  return function *(dispatch) {
    let key = yield dispatch(authenticate(creds));
    // if the dispatch does not return a value, then we can't compose.
    let userData = yield dispatch(fetchAsync(user, key));
    dispatch({ type: "user-done", userData });
  }
}
```

I think I'm missing something or we're not understanding each other, how would we go about making async action creators both composable and stand alone?

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1215251-defer-middleware.patch

Updated, but probably should iron out how to handle composability and stand alone way to call these action creators -- or I guess one way to handle it would be:

```
function asyncThing (d) {
  return function*(dispatch) {
    dispatch({ type: ... })
    return data;
  }
}

function composerTask () {
  return function*() {
    yield asyncThing("abc");
  }
}
```

So the action creator is still dispatching on its own, and the COMPOSER function is not dispatching the previous value.

I like this. I think I'll do this unless there's any objections, but I think solves the previous concerns

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Ah, that's why I didn't like it before. Promise versions resolve to the dispatchable actions. So you'd have to know the implementation of the action creator to handle it in a composing function.

Ugh.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9)
> ```
> function asyncThing (d) {
>   return function*(dispatch) {
>     dispatch({ type: ... })
>     return data;
>   }
> }
> 
> function composerTask () {
>   return function*() {
>     yield asyncThing("abc");
>   }
> }
> ```
> 
> So the action creator is still dispatching on its own, and the COMPOSER
> function is not dispatching the previous value.
> 
> I like this. I think I'll do this unless there's any objections, but I think
> solves the previous concerns

This is the general idea I had in mind.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8674594 [details] [diff] [review]
1215251-defer-middleware.patch

Review of attachment 8674594 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/redux/middleware/task.js
@@ +27,5 @@
> +        }
> +      }, error => executeSoon(() => {
> +        reportException(ERROR_TYPE, error);
> +        dispatch({ type: ERROR_TYPE, error });
> +      }));

Split out the duplicated error handler into a single function instead of having copy-pasted code?

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1215251-defer-middleware.patch

Got rid of the promise middleware stuff -- that's not related to this, I'm not sure why I thought it was.

So the result is now we can dispatch async generators from a composer, but we're not actually dispatching the return value, we're passing in dispatch to the other async creator. That seems to do the trick of abstracting sync/async and being composable and independent. The dispatch still returns the return value of the action creator (a promise) which resolves to the return value of the generator. This could return another action if desired, and the composer can redispatch if needed.

Could use some follow ups, maybe, but we'll see if we need to in practice.

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

See test_middleware-task-02.js for how to use it, that's a good example of composability

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/743fc988e5d7

Comment 16

[James Long (:jlongster)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)
> 
> I think I'm missing something or we're not understanding each other, how
> would we go about making async action creators both composable and stand
> alone?

What do mean by "stand-alone"? `fetchUser` would be this (note that it dispatches the action itself, but that dispatch is returning the exact same action):

```
function fetchUser (user, key) {
  return function *(dispatch) {
    let data = yield XHR(user, key);
    return dispatch({ type: "fetched-user", data });
  }
}
```

Now this can be called stand-alone (is that what you meant?) AND it can be called from other action creators. I think action creators should not have to dispatch actions created by other action creators; they should be dispatched within the original action creator.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #10)
> Ah, that's why I didn't like it before. Promise versions resolve to the
> dispatchable actions. So you'd have to know the implementation of the action
> creator to handle it in a composing function.
> 
> Ugh.

I'm not sure what you mean. Promise action creators will create a promise that resolve to whatever final value is given in the promise. For example:

function fetchUser(user, key) {
  return {
    type: constants.FETCH_USER,
    user: user,
    key: key,
    [PROMISE]: Task.spawn(function*() {
      let data = yield XHR(user, key);
      let newData = ... // do some processing with data
      return newData;
    }
  }
}

If you did `yield dispatch(fetchUser(user, key))` in another action creator, you should just get `newData`. You should not get an action. Am I wrong?

Generally, this looks like we're trying to just have normal Task function but with a `dispatch` function available. This is fine for high-level flows, but at least in my experience, I don't need them as much as promise action creators. They are better in that they represent a concrete async action, and pump actions through the system. Usually you have a bunch of promise action creators, and they can be composed into higher-level flows if needed (which aren't named by an action type, and don't pump actions to indicate start/finish of the whole flow).

If it would help, please look through my refactor, where I haven't really felt the need for this: https://github.com/jlongster/gecko-dev/tree/debugger-refactor-sources-4/devtools/client/debugger/content/actions

But I can see it being useful, though I think we should be using the promise middleware more.

(Happy to talk more about this tomorrow, I could still be missing something)

Comment 17

[James Long (:jlongster)]

Sounds like you've resolved this in comment 13 with similar comments to what I said above, thought I'd still type that out though.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/743fc988e5d7

Comment 19

[James Long (:jlongster)]

Only downside to this is that `dispatch` will be running inside a promise if you manually dispatch in a task. But that's ok for now. So far I've liked that we could guarantee that the UI is never running inside a promise.

I suspect we'll use this less than you think though. Most action creators should be described as a promise and tagged with an action constant. But we'll see.

Comment 20

[James Long (:jlongster)]

It also doesn't help very much to just dispatch `ERROR_TYPE`, the UI will have no idea what actually errored. All it knows is that a task somewhere errored.

That's why the promide middleware is better: it "tags" the whole process with an action type and args so the UI knows exactly what process failed or succeeded.