Sync docs with code: allow async process and prevent persistent state mutation by side-effects

[corrideat]

Problem

This issue is a follow up on okTurtles/group-income#1544 and #75 (the documentation part) and a related conversation.

We have two separate (but related) issues about the way the process and sideEffect APIs should behave.

1. The documentation says that process should never be async, but that's not what the code currently does, since process does support async. In GI, we currently have two instances of async process, although those could be avoided (however, in a way that would require re-creating groups, if we removed await from @chelonia/lib).
2. The documentation says that side-effects should not modify the state, but currently there's nothing stopping them from doing just that.

The reasoning behind process being strictly synchronous seemed to come from two places:

1. From a consistency standpoint, changes to the state used to be applied in a different way, by mutating the state directly and then reverting changes in case of an error (now, we deep-copy the state and apply changes if the operation was successful). This earlier approach was more fragile and harder to use in async scenarios.
2. From an API design standpoint, process isn't supposed to be used for side-effects. Since many (but not all) use-cases for asynchronous operations imply or are strongly correlated with side-effects, allowing for asynchronous process functions could enable developers to use the function incorrectly (that is, for side-effects), which can result state inconsistencies. Note that synchronous side-effects can exist too, so banning asynchronous APIs doesn't entirely achieve the desired effect of preventing accidentally introducing side-effects into the process function.

The reason side-effects currently can (but should not) modify the state is that the state is passed to side-effects and this is an essential part of the side-effects API. We could provide a read-only view, but doing so might require additional deep-copying.

Solution

Based on the earlier discussion, the agreement seems to be that we can continue to allow process to be async (i.e., keep the code as is) and update the documentation and source comments to say it's strongly discouraged. That addresses the first issue mentioned, regarding an inconsistency between what the code does and what the documentation says.

An alternative solution to the first issue mentioned is to keep the documentation as is and remove await from handleEvent.applyProcessResult (and make this function synchronous). This would be a breaking change, and it would also prevent developers from calling APIs that have deterministic results but are nonetheless defined as async.

Also, based on the same discussion, there's a way that we could address the second issue mentioned without incurring in an extra state deep copy or walking the object. By moving the location where await handleEvent.applyProcessResult.call happens to be right after await handleEvent.processMutation.call but before the 'process any side-effects' block, we can give side-effects a state object that technically can be modified, but the changes wouldn't be persistent (as they are persisted by calling applyProcessResult). However, this change needs to be carefully examined to ensure its correctness and that it won't be breaking.

[taoeffect]

We could provide a read-only view, but doing so might require additional deep-copying.

A Proxy could be used to avoid the copy as one possibility

[corrideat]

A Proxy could be used to avoid the copy as one possibility

Not really, as you'd still need to deep-walk the object to proxy all sub-fields that are objects. This is likely more expensive than deep-copying the whole thing.

ETA: You don't have to pay the cost up-front, as the deep-proxying part can be done lazily. The Proxy approach, however, might not work with certain types of sandboxing, as you can't send a Proxy objects across execution contexts, such as from a service worker to a dedicated worker (OTOH, in these scenarios, you are left with deep-copying the whole thing, as crossing contexts clones the object; not sure if maybe browsers do COW in these cases).

[taoeffect]

K, in that case we could also do as discussed and make it a configuration option as a debug tool to devs to check for bad modifications when enabled

[corrideat]

we could also do as discussed and make it a configuration option as a debug tool to devs to check for bad modifications when enabled

We could, but if we get the same effect from moving code around, I'd prefer that option. Having extra options to enable can be helpful, but it complicates the DX and having things behave differently based on what options are enabled can introduce subtle bugs (see next paragraph; the meaning here is that a deep copy that's passed to the side-effect doesn't prevent the changes, only hides them, and so can potentially introduce / hide bugs instead of finding them).

If we go for the dev option at all, I'd rather not deep clone (this doesn't guarantee no mutations, only makes them invisible) but go with the Proxy solution (making the side effect throw if the state is modified at all)