Async/Await Support #176

mikew · 2017-10-02T13:48:21Z

This PR adds support for native async / await functions.

There is a special handler when the promise is a function with a constructor named AsyncFunction (as they are in Node and modern browsers)
In order to support compiled async/await code, it also calls the function and checks if the result is a promise.
When calling the function, it passes dispatch and getState from redux, just like redux-thunk.

So a (maybe silly) example from the guide goes from this:

// fulfilled promise
const foo = () => {
  return dispatch => {

    return dispatch({
      type: 'TYPE',
      payload: new Promise()
    }).then(() => dispatch(bar()));
  };
}

to this:

const foo = () => ({
  type: 'TYPE',
  async payload(dispatch) {
    dispatch(bar());
  }
});

mikew · 2017-10-02T14:03:18Z

Also while working on this I noticed your instanbul config is off, and these builds are silently failing: https://travis-ci.org/pburtchaell/redux-promise-middleware/builds/282257458#L608-L651

Also, this needs yet another babel config so that it stops compiling async/await to its own thing. I've tested that locally but haven't included it in the PR.

pburtchaell · 2017-10-02T14:22:08Z

@mikew Thanks! I'll take a look into this later this week. 👍

pburtchaell · 2017-10-08T14:10:39Z

I noticed your instanbul config is off, and these builds are silently failing

This is now fixed on master. Would you mind rebasing your branch to master, @mikew?

mikew · 2017-10-08T16:40:28Z

Can do!

* origin/master: Remove known vulnerabilities badge Fix Istanbul tests Update dev dependencies

mikew · 2017-10-08T16:55:28Z

I merged instead of rebasing, since a rebase would require push --force.

pburtchaell · 2017-10-08T18:29:57Z

That's fine! Next step here is to add some documentation. Could you add a guide to the docs directory? After that, I'll start on a code review. 👍

pburtchaell · 2017-10-08T19:17:51Z

Just a heads up that you might want to rebase/merge again. I recently pushed some updates to the docs.

…-middleware into async-await-support * 'master' of https://github.com/pburtchaell/redux-promise-middleware: Rename file Update documentation Add comments Update examples Reorder gitignore and npmignore

mikew · 2017-10-09T04:01:36Z

Should I rewrite the chaining actions docs since it's now handled internally?

mikew · 2017-10-09T14:43:32Z

Actually the semantics are just different enough in a couple of examples that, so I think I'll leave them be.

pburtchaell · 2017-10-10T01:55:58Z

Should I rewrite the chaining actions docs since it's now handled internally?

No, I don't think this change warrants removing any content/guides from the docs. I think there should just be a new section for use with async functions. This, and examples where ever else it would make sense. Would you agree?

mikew · 2017-10-12T14:55:41Z

Agreed, and sorry it's taking me so long.

robhowell · 2017-10-26T14:50:07Z

Hi @mikew - this looks like a really useful change, can't wait until it's merged! Would you like any help writing the documentation?

mikew · 2017-10-30T18:31:56Z

Oh gosh that would be lovely. I've added what I have to the PR: https://github.com/pburtchaell/redux-promise-middleware/pull/176/files#diff-596e1efe9b79c33a5a05e9b0fe30f33f

What's there may even suffice.

mikew · 2017-10-30T18:33:01Z

Something that I didn't mention in there, is that when you don't use an async function we will still use that value for the payload.

tomatau · 2017-10-30T18:41:41Z

I'd like to see some clarity about the semantics between the return value (resolve of the async func) and the thunk like dispatch. What's the order that these will dispatch in according to the middleware.

const foo = () => ({
  type: 'TYPE',
  async payload(dispatch) {
    const filteredResult = await getResultAsync()
      .then(result => {
        dispatch(foo(result))
        return filter(result)
      })
    dispatch(bar())
    return filteredResult
  }
});

Here I'd expect 4 actions to be dispatched

pending of TYPE
fulfilled of TYPE
FOO
BAR

What would the order be?

mikew · 2017-10-30T18:44:47Z

Not sure why you'd expect FOO and BAR to be dispatched after TYPE_FULFILLED, if that were the case you'd never be able to use the resolved value from the function in the _FULFILLED action.

tomatau · 2017-10-30T18:46:09Z

@mikew I don't expect that order, I'm asking what the order would be.

But to play devil's advocate, how would I dispatch an action after fulfilled though? If I did want to?

mikew · 2017-10-30T18:47:46Z

The phrase "Here I'd expect 4 actions to be dispatched" is a little confusing then :)

It would be:

TYPE_PENDING
FOO
BAR
TYPE_FULFILLED, with a payload of filteredResult

tomatau · 2017-10-30T18:49:29Z

I came with absolutely no expectations about the order and used an unordered list!... I'm just a wide eyed curious cat asking! I couldn't put them in an order when listing them!

tomatau · 2017-10-30T18:53:24Z

It would be:
TYPE_PENDING
FOO
BAR
TYPE_FULFILLED, with a payload of filteredResult

I feel like the PENDING would happen after the FOO and the BAR because the payload would be invoked by the middleware before the pending was dispatched?...

https://github.com/mikew/redux-promise-middleware/blob/986f650cea8e01b7ff6f0e7905b8fe04822aeac7/src/index.js#L45

mikew · 2017-10-30T18:53:36Z

But to play devil's advocate, how would I dispatch an action after fulfilled though? If I did want to?

You can't, you'd have to use redux-thunk, which gives you 100% control of the flow of actions. I don't think that's any different than this middleware currently works.

tomatau · 2017-10-30T18:55:22Z

You can't, you'd have to use redux-thunk, which gives you 100% control of the flow of actions. I don't think that's any different than this middleware currently works.

Yes, these latest versions work that way...

grumbles to self and continues to support version 2

mikew · 2017-10-30T18:58:23Z

I feel like the PENDING would happen after the FOO and before the BAR because the payload would be invoked by the middleware before the pending was dispatched?...

That's actually a good catch, and I didn't write any specs to verify the behaviour in that case.

mikew · 2017-10-30T19:00:06Z

Yes, these latest versions work that way...

Are you stores dependant on data existing in other stores? That seems like a recipe for disaster. I'd move that to the component layer, and if the data that it requires from two stores isn't available yet, well, you're still loading.

pburtchaell · 2017-11-05T14:54:28Z

const foo = () => ({
  type: 'TYPE',
  async payload(dispatch) {
    const filteredResult = await getResultAsync()
      .then(result => {
        dispatch(foo(result))
        return filter(result)
      })
    dispatch(bar())
    return filteredResult;
  }
});

I feel like the PENDING would happen after the FOO and before the BAR because the payload would be invoked by the middleware before the pending was dispatched?

@tomatau I think it makes sense to dispatch the PENDING action at the time the promise is instantiated (or the async function called). In this case, (unless I'm mistaken) the promise is instantiated when the async payload(dispatch) function is called.

Given the design principle async action objects describe the promise state, this order makes sense to me.

TYPE_PENDING
FOO
BAR
TYPE_FULFILLED (or TYPE_REJECTED)

Let's just make sure this is well documented!

tomatau · 2017-11-05T14:57:22Z

@tomatau I think it makes sense to dispatch the PENDING action at the time the promise is instantiated (or the async function called). In this case, (unless I'm mistaken) the promise is instantiated when the async payload(dispatch) function is called.

The PR would need to be changed for this to be the case.

I feel like this PR/feature is a bit confusing even in a basic example and also a bit restrictive in what you can do with it all in order to support something that kinda looks cool? Unless I'm missing something?

pburtchaell · 2017-11-05T15:08:39Z

The PR would need to be changed for this to be the case.

Yep, lines 44-45 need to be moved to call the async function after the pending action is dispatched on lines 106-110.

I feel like this PR/feature is a bit confusing even in a basic example

In my opinion, at a bare minimum, it does make sense to support async functions, which is where this PR started.

I do agree there is some complexity/confusion, though. I think that comes from (1) providing dispatch and getState as arguments to the async function and (2) understanding the order in which actions are dispatched.

mikew · 2017-11-05T16:18:07Z

The problem is with transpiled async functions. If they're not transpiled, we can tell ahead of time if it's an async function.

But if it's transpiled, we need to call the function to tell if it's async or not. And if we call the function, any actions it dispatches will be dispatched before we can do _PENDING.

mikew · 2017-11-05T16:42:42Z

So I can see a few ways of handling this:

Keep it as is, this way async functions would be treated the same as functions that return a promise, just like Node does
Have special handling of async functions, this would mean async functions are treated differently than functions that return a promise
Ditch the dispatch/getState bits and rely on redux-thunk

pburtchaell · 2017-11-06T22:30:08Z

The problem is with transpiled async functions. If they're not transpiled, we can tell ahead of time if it's an async function.

What's the purpose of this comparison, in that case?

(promise.constructor.name === 'AsyncFunction')

Full disclaimer, I don't use async functions in practice and don't know the ins and outs of browser support and how it's transpiled (assuming this is with Babel?).

I can see a few ways of handling this [...] Ditch the dispatch/getState bits and rely on redux-thunk

What do you think of removing dispatch and getState from this PR and only implementing support for async functions?

const foo = () => ({
  type: 'TYPE',
  async payload() {
    const result = await doSomething();
    return result;
  }
});

This will help move this PR along IMO.

In another PR and/or issue, we can talk about how to introduce dispatch and getState. I should note this is somewhat related to issue #67. It would be great if we could marry this discussion with the discussion there.

mikew · 2017-11-07T00:15:40Z

Yes, Babel or TypeScript will transpile asynchronous functions to regular old functions that return a promise. If they aren't transpiled, you can actually determine if they are asynchronous or not ahead of time, which is what that line is doing.

In retrospect, that line is probably useless, because the other promise check will work. But if you ever wanted to treat them differently, that would be the place.

I'm cool with ditching the dispatch/getState bits, but I'm also one of the people who was never affected by #67, and would just solve it another way.

pburtchaell · 2017-11-13T22:48:44Z

if (typeof promise === 'function') {
  const functionResult = promise();

  if (isPromise(functionResult)) { 
    ..
  }
}

This change makes sense: check if the payload is a function. If true, call the function and check if the return is a promise. I have two concerns though.

First, the typeof promise === 'function' comparison works, but—to play devils advocate here—it works only under the assumption a function will always be an async function. Yes, it is an antipattern to have a synchronous function as the payload, but I think our implementation should be robust enough to handle this case.

// Yes, this is an antipattern...
// ...but, if someone does implement it, it would be called and have unexpected results.
const foo = () => ({
  type: 'TYPE',
  payload() {
    ...
  }
});

Second, we must call the async function to see if a promise is returned.

const functionResult = promise()

This means the function is called before the pending action is dispatched. This this an issue @tomatau brought up earlier when he asked about the order in which actions are dispatched. I don't think this makes sense.

Given this silly example (using Redux Thunk):

const fooAction = () => {
  return dispatch => {

    return dispatch({
      type: 'FOO',
      async payload() {
        dispatch({
          type: 'BAR'
        });

        return true;
      }
    });
  };
};

The dispatched action order is:

BAR
FOO_PENDING
FOO_FULFILLED

EDIT: I should mention, yes, this is the current behavior, but I think if we implement async functions—where someone is more likely to dispatch multiple async actions in a chain—we should change the order in which the actions are dispatched.

So, in conclusion, is there another way to can detect an async function? The (promise.constructor.name === 'AsyncFunction') comparison would be perfect, but as you mentioned, it isn't supported by browsers.

If there isn't another way, I worry we might not be able to implement this until async functions are supported by browsers.

pburtchaell · 2017-11-13T22:49:18Z

Also, I'm sorry for the delay! I was away this past week and just getting back.

mikew · 2017-11-13T23:46:51Z

Yes, it is an antipattern to have a synchronous function as the payload, but I think our implementation should be robust enough to handle this case.

That's what this covers:

https://github.com/mikew/redux-promise-middleware/blob/6d2d0ec5f7d915d3cbeb1e4964fa8567a8825b6f/src/index.js#L51

So, in conclusion, is there another way to can detect an async function?

Nope. In fact it looks like checking for AsyncFunction is a faux pas: tc39/proposal-async-await#78

pburtchaell · 2017-11-16T18:46:50Z

Okay. This is good to merge into the master branch, but I want to do some cleanup work and review all of the tests before it's released to npm. I'll keep you posted on the status @mikew and also leave #175 open until it's released!

Thanks for the work on this and for keeping up with the comments. 👍

mikew added 6 commits September 29, 2017 21:13

WIP: async-await + generator support

e71ebff

do what we can if the function does not return a promise

25dfac8

prefer checking against lastMiddlewareModifies.spy in native async tests

c9f6879

prefer checking if it's actually a promise vs checking the action type

6c3a954

don't treat GeneratorFunction as AsyncFunction

f610559

test both payload and payload.promise

bf0de78

pburtchaell added enhancement in progress Issues in progress by the author(s) labels Oct 8, 2017

pburtchaell assigned mikew Oct 8, 2017

pburtchaell self-requested a review October 8, 2017 13:55

Merge remote-tracking branch 'origin/master' into async-await-support

c43ba98

* origin/master: Remove known vulnerabilities badge Fix Istanbul tests Update dev dependencies

Merge branch 'master' of https://github.com/pburtchaell/redux-promise…

05ba9a7

…-middleware into async-await-support * 'master' of https://github.com/pburtchaell/redux-promise-middleware: Rename file Update documentation Add comments Update examples Reorder gitignore and npmignore

WIP: add async-await documentation

986f650

stop passing dispatch / getState to async functions

6d2d0ec

pburtchaell merged commit ebb1f5f into pburtchaell:master Nov 16, 2017

pburtchaell mentioned this pull request Nov 16, 2017

Develop Async/Await Support #184

Merged

mikew mentioned this pull request Jan 8, 2018

Handle async functions while providing thunk-like functionality #175

Closed

Async/Await Support #176

Async/Await Support #176

Conversation

mikew commented Oct 2, 2017 • edited by pburtchaell Loading

mikew commented Oct 2, 2017

pburtchaell commented Oct 2, 2017

pburtchaell commented Oct 8, 2017 • edited Loading

mikew commented Oct 8, 2017

mikew commented Oct 8, 2017

pburtchaell commented Oct 8, 2017

pburtchaell commented Oct 8, 2017

mikew commented Oct 9, 2017

mikew commented Oct 9, 2017

pburtchaell commented Oct 10, 2017

mikew commented Oct 12, 2017

robhowell commented Oct 26, 2017

mikew commented Oct 30, 2017

mikew commented Oct 30, 2017

tomatau commented Oct 30, 2017

mikew commented Oct 30, 2017

tomatau commented Oct 30, 2017 • edited Loading

mikew commented Oct 30, 2017

tomatau commented Oct 30, 2017

tomatau commented Oct 30, 2017 • edited Loading

mikew commented Oct 30, 2017

tomatau commented Oct 30, 2017

mikew commented Oct 30, 2017

mikew commented Oct 30, 2017

pburtchaell commented Nov 5, 2017 • edited by tomatau Loading

tomatau commented Nov 5, 2017

pburtchaell commented Nov 5, 2017 • edited Loading

mikew commented Nov 5, 2017

mikew commented Nov 5, 2017

pburtchaell commented Nov 6, 2017

mikew commented Nov 7, 2017

pburtchaell commented Nov 13, 2017 • edited Loading

pburtchaell commented Nov 13, 2017

mikew commented Nov 13, 2017

pburtchaell commented Nov 16, 2017

mikew commented Oct 2, 2017 •

edited by pburtchaell

Loading

pburtchaell commented Oct 8, 2017 •

edited

Loading

tomatau commented Oct 30, 2017 •

edited

Loading

tomatau commented Oct 30, 2017 •

edited

Loading

pburtchaell commented Nov 5, 2017 •

edited by tomatau

Loading

pburtchaell commented Nov 5, 2017 •

edited

Loading

pburtchaell commented Nov 13, 2017 •

edited

Loading