RFC: Command abstraction for effects

[charypar]

Note for reviewers

Have no fear! About 1500 of the total lines of this PR is in tests. They do demonstrate the APIs quite well, but hopefully don't require very thorough checking. Another 250 lines is a copy of this PR description (so it stays around).

I've left a number of comments in places starting RFC: - these are places where I have specific questions about the details of this proposal and would love your feedback. Thank you!!

What is this RFC?

This is a proposed implementation of a new API for creating (requesting) side-effects in crux apps. It is quite a significant part of the Crux API surface, so I'd like feedback on the direction this is taking, rather than just hitting merge.

Why?

Why a new effect API, you may ask. Was there anything wrong with the original one? Not really. Not critically wrong anyway. One could achieve all the necessary work with it just fine, it enables quite complex effect orchestration with async Rust and ultimately enables Crux cores to stay fully pure and therefore be portable and very cheaply testable. This new proposed API is an evolution of the original, building on it heavily.

However.

There's a number of paper cuts, obscured intentions, limitations, and misaligned incentives which come with the original API:

The original API is oddly imperative, but non-blocking.

A typical call to a capability looks roughly like: caps.some_capability.do_a_thing(inputs, Event::ThingDone). This call doesn't block, but also doesn't return any value. The effect request is magically registered, but is not represented as a value - it's gone. Fell through the floor and into the Crux runtime.

Other than being a bit odd, this has two consequences. One is that it is impossible to combine or modify capability calls in any other way than executing them concurrently. This includes any type of transformation, like a declarative retry strategy or a timeout and, in particular, cancellation.

The more subtle consequence (and my favourite soap box) is that it encourages a style of architecture inside Crux apps, which Crux itself is working quite hard to discourage overall.

The temptation is to build custom types and functions, pass capability instances to them and call them deep down in the call stacks. This obscures the intended mental model in which the update call mutates state and results in some side-effects - it returns them back to the shell to execute. Except in the actual type signature of update it does not.

The intent of Crux is to very strictly separate code updating state from code interacting with the outside world with side-effects. Borrowing ChatGPT's analogy - separate the brain of the robot from its body. The instructions for movement of the body should be an overall result of the brain's decision making, not sprinkled around it.

The practical consequence of the sprinkling is that code using capabilities is difficult to test fully without the AppTester for the same reason it is difficult to test any other code doing I/O of any sort. In our case it's practically impossible to test code using capabilities withou the AppTester, because creating instances of capabilities is not easy.

The original API doesn't make it obvious that the intent is to avoid this mixing and so people using it tend to try to mix it and find themselves in various levels of trouble.

In summary: Effects should be a return value from the update function, so that all code generating side effects makes it very explicit, making it part of its return type. That should in turn encourage more segregation of stateful but pure logic from effects.

Access to async code is limited to capabilities and orchestration of effects is limited to async code

This, like most things, started with good intention - people find async code to be "doing Rust on hard mode", and so for the most part Crux is designed to avoid async until you genuinely need it. Unfortunately it turned out to mean that you need async (or loads of extra events) as soon as any level of effect chaining is required.

For example - fetching an API auth token, followed by three HTTP API calls, followed by writing each response to disk (or local storage) is a sequence of purely side-effect operations. The recipe is known up front, and at no point are any decisions using the model being made.

With the original API the choices were either to introduce intermediate events between the steps of the effect state machine OR to implement the work in async rust in a capability, which gets to spawn async tasks. The limitation there was that the tasks can't easily call other capabilities.

With the introduction of the Compose capability that last limitation was removed, allowing composition of effects across capabilities, even within the update function body, so long as the capabilities offer an async API. The result was calls to caps.compose.spawn ending up all over and leading to creation of a new kind of type - a capability orchestrator, for example an API client, which is built from a few capabilities (lets say HTTP, KV and a custom authentication) and Compose. This kind of type is basically untestable on its own.

In summary: It should be possible to do simple orchestration of effects without async code and gradually move into async code when its expressivity becomes more convenient.

Testing code with side-effects requires the AppTester

As covered above, the code producing side effects requires a special runtime in order to run fully and be tested, and so any code using capabilities automatically makes testing impossible without the AppTester. And of course apps themselves are only testable with the AppTester harness.

In summary: It should be possible for tests to call the update function and inspect the side effects like any other return value.

Capabilities have various annoying limitations

• Capabilities are "tethered" to the crux core, allowing them to submit effects and events and spawn tasks, but it means instances of capabilities have to be created by Crux and injected into the update function.
• The first point means that capability authors don't have any control over their capability's main type, and therefore it's impossible for capabilities to have any internal state. For capabilities managing ongoing effects, like WebSocket connections for example, this is limiting. It can be worked around by the capability creating and returning values representing such ongoing connections, which can communicate with a separate task spawned by the capability over an async channel. The task read from the channel in a loop, and therefore can have local state. It works, but it is far from obvious.
• They have to offer two sets of APIs: one for event callback use, one for async use. This largely just adds boilerplate, but there is a clear pattern where the event callback calls simply use their async twins. That can be done by Crux and the boilerplate removed.
• They don't compose cleanly. With the exception of Compose, capabilities are expected to emit their own assigned variant of Effect, preordained at the time the capability instance is created. This doesn't specifically stop one capability from asking another to emit an effect, but it is impossible to modify the second capability's request in any way - block specific one, retry it, combine it with a timeout, all the way up to completely virtualising it: resolving the effect using other data, like an in-memory cache, instead of passing it to the shell.

In summary: Capabilities should not be special and should simply be code which encapsulates

• the definition of the core/shell communication protocol for the particular type of effect
• creation of the requests and any additional orchestration needed for this communication

App composition is not very flexible

This is really just another instance of the limitations of capabilities and the imperative effect API. Any transformations involved in making an app a "child" of another app has to be done up front. Specifically, this involves mapping or wrapping effects and events of the child app onto the effect and event type of the parent, typically with a From implementation which can't make contextual decisions.

In summary: Instead, this mapping should be possible after the fact, and be case specific. It should be possible for apps to partially virtualise effects of the child apps, re-route or broadcast the events emitted by one child app to other children, etc.

How does this improve the situation?

So, with all (or at least most of) the limitations exposed, how is this proposed API better?

Enter Command

As others before us, such as Elm and TCA, we end up with a solution where update returns effects. Specifically, it is expected to returns an effect orchestration which is the result of the update call as an instance of a new type called Command.

In a way, Command is the current effect executor in a box, with some extras. Command is a lot like FuturesUnordered - it holds one or more futures which it runs in the order they get woken up until they all complete.

On top of this, Command provides to the futures a "context" - a type with an API which allows the futures within the command to submit effects and events. This is essentially identical to the current CapabilityContext. The difference is that the effects and events get collected in the Command and can be inspected, modified, forwarded or ignored.

In a general shape, Command is a stream of Effect or Events created as an orchestration of futures. It also implements Stream, which means commands can wrap other commands and use them for any kind of orchestration.

Orchestration with or without async

Since Commands are values, we can work with them after they are created. It's pretty simple to take several commands and join them into one which runs the originals concurrently:

let command = Command::all([command_a, command_b, command_c]);

Commands provide basic constructors for the primitives:

• A command that does nothing (a no-op): Command::done()
• A command that emits an event: Command::event(Event::NextEvent))
• A command that sends a notification to the shell: Command::notification(my_notification)
• A command that sends a request to the shell:
  Command::request_from_shell(my_request).then_send(Event::Response)
• A command that sends a stream request to the shell:
  Command::stream_from_shell(my_request).then_send(Event::StreamEvent)

Notice that the latter two use chaining to register the event handler. This is because the other useful orchestration ability is chaining - creating a command with a result of a previous command. This requires a form of the builder pattern, since commands themselves are streams, not futures, and doing a simple .then would require a fair bit of boilerplate.

Instead, to create a request followed by another request you can use the builder pattern as follows:

let command = Command::request_from_shell(a_request)
    .then(|response| Command::request_from_shell(make_another_request_from(response)))
    .then_send(Event::Done);

This works just the same with streams or combinations of requests and streams.

.then and Command::all are nice, but on occasion, you will need the full power of async. The equivalent of the above with async works like this:

let command = Command::new(|ctx| async {
    let response = ctx.request_from_shell(a_request).await;
    let second_response = ctx.request_from_shell(make_another_request_from(response)).await;

    ctx.send_event(Event::Done(second_response))
})

Alternatively, you can create the futures from command builders:

let command = Command::new(|ctx| async {
    let response = Command::request_from_shell(a_request).into_future(ctx).await;
    let second_response = Command::request_from_shell(make_another_request_from(response)).into_future(ctx).await;

    ctx.send_event(Event::Done(second_response))
})

You might be wondering why that's useful, and the answer is that it allows capabilities to return the result of Command::request_from_shell for simple shell interactions and not worry about whether they are being used in a sync or async context. It would be ideal if the command builders themselves could implement Future or Stream, but unfortunately, to be useful to us, the futures need access to the context which will only be created once the Command itself is created.

Testing without AppTester

Commands can be tested by inspecting the resulting effects and events. The testing API consist of essentially three functions: effects(), events() and is_done(). All three first run the Command's underlying tasks until they settle and then return an iterator over the accumulated effects or events, and in the case of is_done a bool indicating whether there is any more work to do.

An example test looks like this:

#[test]
fn request_effect_can_be_resolved() {
    let mut cmd = Command::request_from_shell(AnOperation).then_send(Event::Completed);

    let effect = cmd.effects().next();
    assert!(effect.is_some());

    let Effect::AnEffect(mut request) = effect.unwrap();

    assert_eq!(request.operation, AnOperation);

    request
        .resolve(AnOperationOutput)
        .expect("Resolve should succeed");

    let event = cmd.events().next().unwrap();

    assert_eq!(event, Event::Completed(AnOperationOutput));

    assert!(cmd.is_done())
}

In apps, this will be very similar, except the cmd will be returned by the app's update function.

This API is mainly for testing, but is available to all consumers in all contexts, as it can easily become very useful for special cases when composing applications and virtualising commands in various ways.

Capabilities are no longer special

With the Command, capabilities become command creators and transformers. This makes them no different from user code in a lot of ways.

The really basic ones can just be a set of functions. Any more complicated ones can now have state, call other capabilities, transform the commands produced by them, etc.

The expectation is that the majority of low-level capability APIs will return a CommandBuilder, so that they can be used from both event callback context and async context equally easily.

Better app composition

Instead of transforming the app's Capabilities types in order to wrap them in another app up front, when composing apps the resulting commands get transformed. More specifically, this involves two map calls:

    let original: Command<Effect, Event> = capability_call();

    let cmd: Command<NewEffect, Event> = original.map_effect(|effect| effect.into()); // provided there's a From impl
    let cmd: Command<Effect, NewEvent> = original.map_event(|event| Event::Child(event));

The basic mapping is pretty straightforward, but can become as complex as required. For example, events produced by a child app can be consumed and re-routed, duplicated and broadcast across multiple children, etc. The mapping can also be done by fully wrapping the original Command in another using async

    let original: Command<Effect, Event> = capability_call();

    let cmd = Command::new(|ctx| async move {
        while let Some(output) = original.next().await {
            match output {
                CommandOutput::Effect(effect) => {
                    // ... do things using `ctx`
                }
                CommandOutput::Event(event) => {
                    // ... do things using `ctx`
                }

            }
        }
    });

Other benefits and features

A grab bag of other things:

• Spawn now returns a JoinHandle which can be .awaited
• Tasks can be aborted by calling .abort() on a JoinHandle
• Whole commands can be aborted using an AbortHandle returned by .abort_handle(). The handle can be stored in the model and used later.
• Commands can be "hosted" on a pair of channel senders returning a future which should be compatible with the existing executor enabling a reasonably smooth migration path
• I havent looked at implementing any, but this API should in theory enable declarative effect middlewares like caching, retries, throttling, timeouts, etc...

Limitations and drawbacks

I'm sure we'll find some. :)

For one, the return type signature for capabilities is not great, for example: RequestBuilder<Effect, Event, impl Future<Output = AnOperationOutput>>. The command builder chaining is using dirty trait tricks, and as a result requires the CommandBuilder trait to be in scope - not completely ideal (but the same is true for FutureExt and StreamExt).

One major perceived limitation which still remains is that model is not accesible from the effect code. This is by design, to avoid data races from concurrent access to the model. It should hopefully be a bit more obvious now that the effect code is returned from the update function wrapped in a Command.

I suspect there are ways around this by storing some Arc<Mutex<T>>s in the model and cloning them into the effect code, etc., but do so at your own peril - tracking causality will become a lot more complex in that world.

Open questions and other considerations

• I have not fully formed a migration plan yet - it should be possible for the two APIs to coexist for a few versions while people move over
• The command API expects the Effect type to implement From<Requst<Op>> for any capability Operations it is used with. We should probably allow these trivial implentations to be macro derived
• I have not fully thought about back-pressure in the Commands (for events, effects and spawned tasks) even to the level of "is any needed?"
• I am not super sure about my implementation of task cancellation using atomics, because they break my head. Help.

I thank and salute you for reading all the way down here. If you have, I 100% owe you a beer - tell me "I've read your entire RFC treatise, and I'm here for my free beer" next time you see me in or near a pub :).

[charypar]

@adwhit @arendjr @olliebatch I'd love any feedback if you can spare the time to review this 🙏🏻

[arendjr]

This is great stuff! I've read the description (so it seems that qualifies for the free beer 😉 ) , and from the sounds of it the priorities are well-explained, and as you've probably guessed I'm very aligned with this!

The first thing that came to mind: It sounds as if the construction of a Command can still be done somewhere deeply nested inside an update() call. I don't think that's necessarily bad, so at first glance this could be fine. But I suspect it can lead to quite messy codebases if people start doing so extensively, so I think documentation-wise it would be good to offer guidance to place all command creators in a common module, such as commands. This way, people have a more organised way of creating their own "command APIs" (I see a bit of similarity with the "thunks" in Redux), that will also make it easier to spot where reuse may be sensible. And it definitely feels cleaner to move all the side-effect chaining/handling out of the otherwise pure code.

For the rest I'll let it sink a bit, and I'll have a peek at the code later. Nice work!

[charypar]

@arendjr Thanks for looking!

It sounds as if the construction of a Command can still be done somewhere deeply nested inside an update() call. I don't think that's necessarily bad, so at first glance this could be fine.

It is certainly possible, and I'm hoping that the hassle of bubbling those back up to be returned will push people away from that design a bit more strongly than the current API (where it's relatively pain-free to do so). Definitely aggree that offering suggestions for patterns in documentation would be good. Thanks for the ideas around organising the codebase, we'll work them in when it comes time to write proper guides for this.

[adwhit]

Do you have an example of how the App trait will change? Presumably the update function becomes

    fn update(&self, event: Self::Event, model: &mut Self::Model, caps: &Self::Capabilities) -> Command<Self::Effect, Self::Event>;

Does the Compose capability go away with this change? I suppose I would like to see a short example to get a feel for this change and also how it is different to #282

[charypar]

Yup, I expect that's how the App trait will evolve as the first step of the transition. I expect Compose will go away at the same time as the caps argument to update disappears towards the end of the transition.

With this PR, I want to first align around how this new API should work, and then separately solve the migration challenge, which is pretty chunky in its own right.

I do imagine the move will end up looking similar to #282, but as much as we can I'd like to make it multiple steps - first enabling the new API to be used (perhaps with a feature flag or simultaneously somehow), adding capability support for them, then documenting the migration path thoroughly and deprecating the old APIs and finally removing them later down the line and tidying up.

I know sizable codebases exist using the original API, so as much as we can I'd like them to be able to make the change over a period of time, not force them to wholesale adopt it or be stuck on an old version.

[charypar]

Ah, the other expected change I forgot is that App will no longer ask you to declare Capabilities and instead will ask you to provide an Effect type. For the crossover, we'll need to make sure the generated Effect type is compatible with this.

I might actually write a little example integration test with a new version of the App trait in it, to make the shape a bit clearer 🤔

[charypar]

Added a demo test in 223609e.

[charypar]

Pushing that test a bit further made me realise that .then might not have the most useful type signature - it doesn't allow any early termination in the chain.

Maybe a .then_try or a .then_filter/.then_maybe is needed?

[arendjr]

Looking a bit deeper into this, I'm still contemplating this thing you mentioned among the downsides:

One major perceived limitation which still remains is that model is not accesible from the effect code. This is by design, to avoid data races from concurrent access to the model. It should hopefully be a bit more obvious now that the effect code is returned from the update function wrapped in a Command.

While I understand why the limitation is there, I'm still wondering whether you can't find some middle ground here. After all, during an async flow you may very well wish to see whether anything changed as part of the model that could be influential as to whether and/or how you should continue your flow. I think right now your only solution would be to send an event, and let the update() call for that event return a new Command, causing an unfortunate "mental jump" in your flow.

I'm wondering whether it would be feasible to create something akin to a get_state() to your context:

let command = Command::new(|ctx| async {
    let response = ctx.request_from_shell(a_request).await;
    let should_send_second_request = ctx.get_state(|model| inspect_model(model));
    if !should_send_second_request {
        return ctx.send_event(Event::Aborted);
    }
    
    let second_response = ctx.request_from_shell(make_another_request_from(response)).await;

    ctx.send_event(Event::Done(second_response))
})