Async performAction

[pkamenarsky]

So a fundamental use case I have goes like this:

performAction ... ->
  -- set loading to true (i.e. show progress bar)
  -- load data
  -- set loading to false, set data in global state

There's purescript-thermite-aff, but the current design doesn't play well with nesting async performActions - check out this ticket.

So basically, I converted PerformAction to be a Producer. The difference in usage is negligible (i.e. use emit \st -> st {...} instead of just \st -> st {...}), but the outcome is much better composability.

Would you be interested in going forward with this? I have to decide on a purescript framework for an ongoing project, and so far thermite is my top choice, but async composability is a must-have.

[born2defy]

Hey - your last comment in the other ticket was that you weren't sure if this would break react interoperability. Have you already verified that this is not the case? Because I would love to see nested async supported.

[pkamenarsky]

Yes, as far as I can see it doesn't break anything - I already converted a sizeable chunk of a halogen app to async thermite and everything works like a charm. You can pull the branch and try for yourself :) If you do and find something that doesn't work please report back!

[born2defy]

I'll take it for a spin this weekend and let you know if I run into any unforced errors.

[paf31]

I started implementing thermite-aff because I wanted this sort of functionality, but I wanted to keep it separate from thermite, as a library for advanced users. Also, I think it's nice to keep the dependencies minimal.

The Producer approach seems like it should be equivalent to the existing approach (in the sense that there should be functions between Producer a (Aff eff) Unit and (a -> Eff eff Unit) -> Eff eff Unit in both directions), so this should be possible as a separate library too. Is it not?

[pkamenarsky]

Oh, I wasn't aware of that equivalence! But I have to admit it's still not clear to me how to turn (a -> Eff eff Unit) -> Eff eff Unit into Producer a (Aff eff) Unit. In other words:

performAction = asyncMany handler
where handler action props state = performAction ???

The implementation of focus doesn't help, since asyncMany captures k here so it can't be passed on to the nested performAction.

[pkamenarsky]

Also I often find myself wishing for the following pattern:

performAction ... = do
  emit \st -> st { .. }
  -- wanting to read and do something with *already transformed state*
  emit \st -> st { .. }
  -- etc

This is a very real use case - for example clearing an array before calling an async function to append some items to said array. Right now, the second function would read the current (and not the transformed) state, thus restoring the cleared items (or else resort to explicitly keeping track of the transformed state and passing that to the next function etc etc)

What would you say about a free monad that supports get in addition to emit/modify, similar to Halogen? I understand that you want to keep the API simple, so what about this: simpleSpec can wrap a pure State -> State function in a modify - so there would be no change to the current public API. For people that need the additional functionality there'd be asyncSpec or something.

[paf31]

Doesn't thermite-aff handle that use case? You should be able to work in the Producer monad and write pretty much what you wrote there.

[pkamenarsky]

I'm sorry if I'm thick and missing the obvious, but could you tell me how?

I fail to see how to

• call a nested performAction from asyncMany (specifically from asyncMany, without using the focus etc combinators)
• read the current state in the Producer monad - it only emits state transforming functions, so how would you access the current (transformed by a previous emit) state?

[paf31]

Ah, ok, I see now.

Perhaps Producer isn't the right tool here then. I'll have a think about this, but maybe Transformer is a better fit.

React could be described by a coroutine with suspension functor given by

data IndexedStore i o a = IndexedStore o (i -> a)

type ReactState state = Co (IndexedStore state state)

emitGiven :: (state -> m state) -> ReactState state m Unit

which zips with a Transformer state state coroutine.

[pkamenarsky]

Cool, I'm gonna try to come up with something too.

[paf31]

@pkamenarsky I put this together, let me know what you think. I think it's a better fit, and I'll put together a PR to use CoTransformer in thermite-aff if it looks okay.

purescript-contrib/purescript-coroutines#10

[pkamenarsky]

Just checking if I got this right: React, acting as a CoTransformer yields a value in a loop, performAction transforms the value and yields it back to React. Is this correct?

In case I understood correctly, the problem with intermediary states still remains:

performAction ... = do
  trasnform \st -> f st
  lift $ affAction currentState
  ...

Now there would be no way to pass f st as currentState to affAction, as far as I can see.

Maybe if transform took i -> m o instead of i -> o it would work, but in that case maybe a get/modify free monad would be simpler.

[paf31]

Yeah, I tried i -> m o but the types don't work out.

I think we'll need a custom coroutine type after all.

[pkamenarsky]

How about this? It's just a draft though, needs to be cleaned up etc.

[paf31]

This looks really great, but I wonder if we should put it in thermite-aff. Thermite used to define an Action monad a bit like this, but it was quite restrictive, and beginners found it confusing. I worry it might be a lot for beginners to learn. The use of lens, by comparison, is totally optional, where this would be required learning.

Two minor comments:

1. Does it make sense to implement Get and Put as primitive and modify as a helper function instead? Maybe not, since modify is almost always the right thing.
2. Why call it Query?

[pkamenarsky]

Query is a bad choice, I agree. Action sound a lot better.

Regarding put vs modify - it's trivial to implement one in terms of the other, so it's your call. I've noticed that I almost always use modify and never put.

The fundamental problem with putting this in thermite-aff (that I also mentioned before) is that I can't seem to come up with a way to convert

((s -> s) -> Eff eff Unit) -> Eff eff Unit

to

Co (Action s) (Aff eff) Unit

(The reverse conversion is trivial). Unfortunately there would be no point to introducing Action if there was no such conversion, because then nesting performActions would be impossible (i.e. no way to call performAction from asyncMany).

Can you come up with something? Then we can just patch thermite-aff instead.

What was the biggest difficulty for beginners with the old Action monad? Maybe we can come up with something to make it easier for newcomers.

[paf31]

Have you seen the aff-coroutines library? I think that might be helpful.

I could be convinced to add this to Thermite itself, if we can make things simple for beginners. From what I remember, it wasn't any one particular thing which made it difficult, it just wasn't clear how to implement certain things.

[paf31]

I'd like to get this merged, ideally before the next release, which will be to update the core libraries dependencies to 1.0. Would you be interested in finishing this off? There isn't a massive rush, since other dependencies still need to be updated. If so, I'll do a more thorough review of this during this week.

After some more thought, I'm less concerned if the coroutine functionality is implemented here as opposed to inside thermite-aff.

Thanks!

[pkamenarsky]

Sure, tell me what is needed still.

I'm using it in production, so it works. Documentation, names, API?

[paf31]

Can this be removed?

[paf31]

Please add documentation for any exported members.

[paf31]

What is your experience so far with this choice of coroutine type? I'd like to think about it some more, but I wonder if there are any alternatives. Without having tried it, it seems to make sense.

I'd still like to merge this before I cut 1.0. Thanks for working on this!

[pkamenarsky]

It works well in my experience. It's like the State monad, so it's pretty intuitive and well known.

What would alternative solutions look like in the presence of async side-effects? I'm not able to think of something simpler short of some sort of FRP, but then thermite would look quite differently on a conceptual level compared to Elm/React.

I'll work on the issues you raised over the weekend and next week. writeStateWithCallback needs to go in purescript-react unfortunately.

[paf31]

@pkamenarsky I've pushed some changes to master to make things compatible with 0.9.1, so this PR will need to be updated, but I'm going to spend some time this afternoon trying out some ideas with coroutines, and I might end up merging this by hand.

[pkamenarsky]

@paf31 Allright, tell me if you need anything!

[paf31]

So I spent a bit of time on this, and I'm a little uncomfortable with the need for Monoid constraint on split, and also the const appearing in foreach now. To me, this says Get is incompatible with these focussing operations, since the state can be updated during an asynchronous operation, and might not match the Prism/Traversal in use any more.

I'm tempted to say we can add in a Maybe to the result of Get and see if that helps, but that seems a bit like a hack.

Perhaps it's better to figure this out, and make a 2.0 release if we can. Let me think on it a bit more.

[pkamenarsky]

What about something like that to make the types work out:

    for_ (sts !! i) \st -> case f i of Spec s -> bimapFreeT (mapQuery (\st' -> fromMaybe st $ st' !! i) (modifying i)) id $ s.performAction a p st

st' !! i could only be Nothing if the subcomponent somehow alters the parent component state, which shouldn't be possible.

[paf31]

st' !! i could only be Nothing if the subcomponent somehow alters the parent component state, which shouldn't be possible.

I think it is possible, since another component can modify the state asynchronously. If another component gets there first, that index might not exist in the list any more.

We need to pick a strategy for dealing with this problem generally. Some strategies might be:

• Remove split and foreach, since they interact poorly with get in general (but I think focus is fine). I don't particularly like this option.
• Have get return a Maybe state, since the original state might not be there any more. I also don't like this option, since the state might be there, but not the one you thought of. For example, a component using foreach might click an Update button on item i, but while that AJAX request was being handled, a websocket message arrived deleting message i - 1. Now what you thought was item i is in fact the old i + 1, and you've updated the wrong item. (This is admittedly also an issue with modify)
• Remove get. This is my favorite option, because I don't think handlers should need to access the state after they start working, but maybe I'm not understanding a use case.

[pkamenarsky]

Wait, actually, isn't this a problem with modify/emit as well? While an async action is running, the state might have been altered, which means that a consequent emit might too modify the wrong index.

What about providing a keying function to foreach i.e. (state -> String) or something, that will then be used to uniquely identify the modified state. If the state doesn't exist anymore, do nothing.

EDIT: alternatively provide a Map String state instead of List state.

[pkamenarsky]

The use case for get is mainly composability, i.e. the ability to write self-contained functions that operate on the current state in Thermite without having to explicitly thread the modified state through all functions (i.e. a paginating function that needs the last element in a list to fetch more results, but that element may have been fetched by a previous modify). Maybe a State monad will help with that though, get doesn't have to necessarily be part of the free monad.

[paf31]

Wait, actually, isn't this a problem with modify/emit as well?

I suppose it is.

We can document this behavior, and users can add some sort of version field to their state to handle it if they need to.

I suggest we could go with the producer approach for now until we figure out the best story for get. We can always add support for it later if we hide the implementation behind a newtype.

My concern is more over the const st than the index changes, honestly. That means get is basically useless inside foreach anyway.

[pkamenarsky]

But that's what I mean, isn't emit useless inside foreach as well? So I guess the problem is with foreach rather than with get. Or maybe I'm misunderstanding something.

A Map or an unique ordering function would solve this problem for both get and modify in the context of foreach.

[paf31]

So, we can handle the race conditions in documentation, I think. The bigger issue, as I see it, is that the const st is incorrect inside foreach. If you modify then get, you won't see the changes. Without making get partial, I'm not sure how to address that.

This is why I'm tempted to put the Producer version in and release 1.0, and worry about get later.

[pkamenarsky]

If we mention this in the documentation, then what about my earlier suggestion (\st' -> fromMaybe st $ st' !! i)?

[paf31]

That should work.

I'll have another look at the Get/Modify approach later, with that in place.

It sort of saddens me to lose the simplicity of producer $$ consumer though 😄

One thing I'm still not sure about though - if we make the assumption that there will only be one asynchronous request at a time per foreach (otherwise, we'll see these sorts of races), then that reduces the need for get, since the only changes to the state during that PerformAction should come from its modify calls, so that action should already know everything needed to decide what to do next.

[paf31]

I've merged CoTransformer into coroutines now, maybe that can be useful too.

[paf31]

I'm going to try implementing this using Transformer and CoTransformer. The idea is that PerformAction can be defined in terms of CoTransformer (Maybe state) (state -> state), where the Maybe will indicate that the optic did not match, or something else went wrong while trying to update.