[WIP] BUG: 'Long-lived' setState function fails to persist updates

[bryphe]

Issue: It seems that there is a 'state persistence' issue - state updated higher in the hierarchy can pave state lower in the hierarchy, when state lower in the hierarchy is set via a 'long-lived' setState handler. I hit this issue when I was working on the slider prototype control in Revery.

It was a little tricky to reproduce in isolation, but I believe there are a couple of conditions required to hit this bug:

• Some code 'hangs-on' to a setState type function acquired from useState. This could take a couple forms - a useEffect that sets condition=Effects.MountUnmount (so it is only fired in the initial mount), or an event handler like in the Mouse.setCapture case. The initial mousedown successfully triggers a state update, but subsequent state updates called during the mousemove events are ignored.
• Elements higher in the hierarchy are updated after the state update. In revery, this happens due to the UI update that occurs in the render function, or when an ancestor component calls a function in response to an event from a lower component (ie, a component listening to onValueChanged from a slider).

I created a test case so far in the PR to 'exercise' this case - I believe if we can get the test case fixed, it'll also address the bugs I saw with the slider functionality. The new test runs the following in sequence:

• Trigger an outer component to have state 5
• Trigger an inner component to have state 6
• Trigger an outer component to have state 7

We'd expect at the end that the inner component would have state 6, but it actually reverts back to its original value of 2 after the last outer component update.

Defect: My understanding so far is that, when code 'hangs on' to a setState function provided by useState, in the scenario outlined above - what happens is that when the handler down the road calls the old setState, it will update a detached instance that isn't up-to-date.

Potential Fixes: I think the crux of the issue is here:

let currentContext = ComponentState.getCurrentContext(globalState);

    let dispatch = (context: ref(option(instance)), action: 'action) => {
      let newVal = reducer(getState(), action);
      updateState(newVal);
      switch (context^) {
      | Some(i) =>
        let {rootNode, component, _} = i;
        Event.dispatch(i.container.onBeginReconcile, rootNode);
        let _ =
          reconcile(rootNode, Some(i), component, i.context, i.container);
        Event.dispatch(i.container.onEndReconcile, rootNode);
        ();
      | _ => print_endline("WARNING: Skipping reconcile!")
      };
    };

    (componentState, dispatch(currentContext));

In that the currentContext is out-of-date in this particular case.

There is some awkwardness in how we manage the state today:

module Make = (StateContextImpl: StateContext) => {
  type t = {
    context: ref(ref(option(StateContextImpl.t))),
    mutable currentState: HeterogenousMutableList.t,
    mutable newState: HeterogenousMutableList.t,

(this concept is overloaded - we use the context to store the entire component instance, and then there are these separate fields looking at currentState/newState - it makes it hard to understand the chain of operations here).

I'm considering a refactor where we give each instance a unique instance ID. Then, instead of currying the entire context to dispatch, we could dispatch this ID, which should persist across re-renders as long as the instance is still available. Then, the state management becomes a hash table of uniqueId -> instance, and managing our hierarchy of instances means managing a tree of uniqueId. I believe this could simplify the state management of instances while addressing the bug exposed by the test case - the setState could never refer to an old node unless it had been removed from the tree (in which case we could detect it and log a warning).

[jchavarri]

@bryphe I wonder how this issue and the internal implementation of state would be affected by the idea discussed in #39 (comment).

If we pass the use* handlers to the callback that createComponent expects, then we could index the global state by component id. Then the chances of overlap would be reduced. I'm not sure this is related, as your comments suggest the problem is with the instances themselves, not with the components. 🤔

Another idea before trying a refactor is to move the ComponentState.getCurrentContext call inside the dispatch, so we guarantee the state is always the latest. Not sure if that would fix the problem.

---

I'm considering a refactor where we give each instance a unique instance ID.

Maybe there are ways to process instance state in a "non-global way", so we can avoid the indirection of keeping instance IDs and a table to store them. I'm a bit concerned about adding IDs to the instances and a new table because I've had painful experiences in the past due to solutions I designed that way and the implications they carried: a new class of situations with "orphan IDs" without a backing value did emerge, the 1:1 relationship between elements and instances could potentially be broken, and in general it involves more work to keep those IDs and the contents of the table updated. I would like to understand better why this problem is happening. Thanks for creating that test!! I have to take a closer look at it. 🙂

This is just an early sketch of an alternate idea, and it is probably out of scope 😅 But, considering right now "elements" in revery are rather "promises of elements", in the sense that <MyComponent /> doesn't develop even one level of the element tree, but returns the function that does so, we could take advantage of that "element laziness" to do the following:

Instead of elements storing a render function in the form of unit => elementWithChildren like here:

reason-reactify/lib/Reactify.re

Lines 8 to 13 in d3271aa

	and render = unit => elementWithChildren
	and component =
	| Primitive(ReconcilerImpl.primitives, render)
	| Component(ComponentId.t, render)
	| Provider(render)
	| Empty(render)

We could rather have this internal render function receive any required data that is needed for the hooks (state, effects etc).

So, things like __globalEffects and maybe __globalState could be "local" instead.

For that to happen, we would probably need to make useState and useEffect know somehow what is the "instance state" or the "instance effects" variables they have to operate in. For that, we would probably need to adapt the component signature to add an extra param in the end...

module CounterButtons = (
  val component((render, ~children, (), {useState, useContext}) => render(() => {
    </view> /* Use useState and useContext here */
  }, ~children))
);

That way, the calls to createElement would remain partially applied until we pass a record with all the hooks function in them. Because this could happen at instantiation-time, those functions could already get as first param any contextual data needed for that instance (kind of like it's done with dispatch and context in useReducer.

I hope that makes sense! There are many unclear parts, and things that could be improved (like the orphan unit in the component callback signature). Curious to know your thoughts!