Better async actions support

[ForsakenHarmony]

This allows you to return objects from async functions/promises to be merged into state the same way as you would in a sync function

This would allow you to write

export default {
	state: {
		result: null,
		error: null,
	},
	actions: {
		login: () => async () => {
			try {
				return {
					result: await authProm,
					error: null,
				}
			} catch (error) {
				return {
					error
				}
			}
		}
	},
};

instead of

export default {
	state: {
		result: null,
		error: null,
	},
	actions: {
		login: () => async (state, actions) => {
			try {
				actions.setResult(await authProm);
				actions.setError(null);
			} catch (error) {
				actions.setError(error);
			}
		},
		setError: error => ({ error }),
		setResult: result => ({ result }),
	},
};

[codecov]

Codecov Report

Merging #558 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #558   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         136    141    +5     
  Branches       41     44    +3     
=====================================
+ Hits          136    141    +5

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)	⬆️

---

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 e713abf...b9b85bd. Read the comment docs.

[jorgebucaran]

@ForsakenHarmony What do you suggest we do when you depend on the current state to compute the next state, but the state you refer to inside the resolving promise has already gone stale?

[ForsakenHarmony]

Wouldn't we have the same problem with the current way of doing things?

[infinnie]

See https://github.com/hyperapp/hyperapp/blob/master/docs/tutorials/gif-search.md

[ForsakenHarmony]

You mean me?

[jorgebucaran]

@ForsakenHarmony Wouldn't we have the same problem with the current way of doing things?

Nope. Because you always have a fresh copy of the state inside an action. It does mean you usually need two actions when dealing with async side-effects.

[infinnie]

Remember that in 0.1x it used to support that feature.

[ForsakenHarmony]

Could still use the current way if you need the current state, not like that falls away

[jorgebucaran]

Yes, we used to handle this, but it adds a lot of complexity and it was removed (among other things) for the 1.0 cut.

[ForsakenHarmony]

If you look at the gif example it needs an extra action to set the url, which seems unnecessary, it doesn't do anything with the current state

[infinnie]

That one could hook HyperApp to turn async actions into function calls so that effectively we need only one action.

[jorgebucaran]

Here is an example of the problem.

const state = {
  count: 0
}

const actions = {
  down: () => state => ({ count: state.count - 1 }),
  up: () => state => ({ count: state.count + 1 }),
  upAsync: () => async state => {
    await delay(1000)

    // The `state.count` may be out of date already!!
    return state.count + 1
  }
}

const view = (state, actions) => (
  <main>
    <h1>{state.count}</h1>
    <button onclick={actions.down}>-</button>
    <button onclick={actions.up}>+</button>
    <button onclick={actions.upAsync}>+</button>
  </main>
)

app(state, actions, view, document.body)

[jorgebucaran]

And here is an example of how we solved it as well:

const state = {
  count: 0
}

const actions = {
  down: () => state => ({ count: state.count - 1 }),
  up: () => state => ({ count: state.count + 1 }),
  upAsync: () => async state => {
    await delay(1000)

    // But returning a function that expects a fresh state is OK!
    return state => state.count + 1
  }
}

const view = (state, actions) => (
  <main>
    <h1>{state.count}</h1>
    <button onclick={actions.down}>-</button>
    <button onclick={actions.up}>+</button>
    <button onclick={actions.upAsync}>+</button>
  </main>
)

app(state, actions, view, document.body)

[infinnie]

So since Promises and asynchronous actions are so important in any practical application you at least provide us with a way that we could add that back.

[ForsakenHarmony]

@jorgebucaran as said before, if you need the current state you can dispatch more actions, but having to write setters for everything is a slight mess imo

[infinnie]

At least state fragments are usually not changed that way, practically speaking

[jorgebucaran]

Maybe we can ignore the problem of the state and recommend that, if you need the current state to compute the next, you use a custom getState action or call another action (as we do now) in that situation.

I remember @SkaterDad being in favor of this partial approach.

/cc @SkaterDad @zaceno @okwolf

[infinnie]

But the current solution is much uglier in my opinion.

[jorgebucaran]

Also, @Mytrill can you chime in? 🙏

[infinnie]

Maybe with a helper function?

[Swizz]

You know, I worked a lot about this feature, along with @ngryman and @jorgebucaran.
We tried a bunch of things #305 #317 #338 #347 #450

So, I am on your side regarding the fact to have to write both actions for async and reducers. I am pretty sure we can found a way to do it. #450 was personnaly my best candidate for it.

[zaceno]

My preference would be not to include this PR, because I much prefer the current behavior of actions simply returning the promise, so that a calling action can await whatever it resolves to, and perform further actions based on that.

I can see how the proposed behavior could be convenient in some cases. But wouldn't a userland solution such as the following work almost just as well?

actions: {
  reduce: fn => (state, actions) => fn(state, actions),

  someAction: ()  => (state, actions) => {
     fetch('...')
     .then(body => body.json())
     .then(data => actions.reduce(state => ({
        foo: state.foo + data.foo,
        bar: data.bar,
        fetching: false,
     }))
     return {fetching: true}
  }
}

[jorgebucaran]

[infinnie]

But uglier…

[jorgebucaran]

@infinnie Why ugly? It's creative & clever. Good job @zaceno!

[ForsakenHarmony]

Even if you add this, you can still use that reduce function if you need the current state, I don't see why it's bad

[jorgebucaran]

@ForsakenHarmony It's problematic, as exemplified here. @zaceno's solution is simple, userland and even fun to write.

Your proposal is not really new, we've had it implemented before as some people already mentioned in the comments.

I'll keep this open for a bit longer, as I want to gather more feedback though.

[ForsakenHarmony]

imo that's for the user to handle, but alright

[SkaterDad]

@zaceno My preference would be not to include this PR, because I much prefer the current behavior of actions simply returning the promise, so that a calling action can await whatever it resolves to, and perform further actions based on that.

I agree with this, 100%.

Way back when hyperapp had events you could hook into, we had a way to do what this PR suggested. I thought it was very convenient, and structured all of my data fetching around it. While it was convenient, it made the app more confusing to debug, and made chaining other actions difficult.

Knowing that hyperapp won't merge any state without explicitly being told to via a simple synchronous action makes the code more predictable and harder to introduce accidental bugs.

[infinnie]

@jorgebucaran sorry that was just my opinion.

[infinnie]

And I read that again. That was awesome. I used to write an indicator jQuery plugin just for indicating that an asynchronous action has begun.

Sorry for my misunderstanding of the code.

[selfup]

@zaceno holy shit 🙏

[okwolf]

I know I'm late to the party, but I brought you a new HOA as a housewarming gift 🎁

I call the little guy withAsync, and he can deal with actions which return a Promise by using the resolved value to update the state including an optional update form where you provide a function that works like our old thunks did. He's even such a little hipster that he supports generators and other iterator factories (including yielding Promises for mixed sync/async goodness).

Here's a different way of writing the @zaceno example using withAsync (notice how it reads in the order of state updates):

const actions = {
  someAction: ()  => function*(state) {
    yield { fetching: true }
    yield fetch(...)
      .then(body => body.json())
      .then(data => state => ({
         fetching: false,
         foo: state.foo + data.foo,
         bar: data.bar
      }))
  }
}

Now to prove that this isn't all just smoke and mirrors, I threw together an updated version of the GIF search using this newborn dude: https://codepen.io/okwolf/pen/EoeyZW?editors=0010

For myself, I'm going to skip all this complexity and stick with treating effects as data using hyperapp-effects to solve my async problems 😊 🔌

[jorgebucaran]

Hacker!

[Mytrill]

I am also for not merging this PR, my reason is that often, for async actions in particular, a single user interaction should trigger multiple async updates in different state slice and you want to gather all the results, and only if successful, update each slice with it's own result, so for me it is a good feature that a user can return a promise containing an object and not automatically merge this object in the current state slice.

[jorgebucaran]

@Mytrill How do you handle this in frapp by the way?

[Mytrill]

In frapp, you have to call the update() function to set the state, otherwise whatever is returned by the action (for frapp, it's just a method) is not processed.

[ForsakenHarmony]

it is a good feature that a user can return a promise containing an object and not automatically merge this object in the current state slice.

but if you want that, does it have to be a state action?

[infinnie]

Then @zaceno, your solution means that someAction might well be some non-action function:

var someFunc = (reduce) => {
   reduce(() => {fetching: true});
   fetch('...')
     .then(body => body.json())
     .then(data => reduce(state => ({
        foo: state.foo + data.foo,
        bar: data.bar,
        fetching: false
     }));
};

Then just let a component pass the reducer down when someFunc() gets called.

[infinnie]

And ordinary functions could be dynamically injected just the good old way.

[jorgebucaran]

Okay, I am just starting to realize @ForsakenHarmony and @infinnie are not the same person.

[ForsakenHarmony]

👀

[infinnie]

0.0