fix: adds a temp cache of node heights to fix node flickering

[codemile]

This PR closes #277 by uses a memory "cache" to store node heights. These heights are then used by the UnknownNodeBody component when the body is not yet available. This resolves the reported issue, because the components are unmounted and mounted when being moved in the DOM structure from the canvas to the dragging list and then back again. The flickering happens because the body component is rendered async and isn't ready on the first render pass. The cache acts as a "last known height" value when the body is not ready.

The fix is a bit hacky in nature. I tried my best to isolate it to separate hooks, and this approach seemed to have the smallest footprint of changes. It did require a "cache" prop to be prop drilled down to the NodeBody component. There is a nodes.reduce() call that rebuilds the cache when nodes change to prevent memory leaks. The performance seems to be fine as the the nodes dependency only changes when nodes are selected.

Note: I couldn't figure out how useUnknownNodeComponentDescriptorFor worked, and only applied the height fix to the UnknownNodeBody component. It's possible other node types could use this fix as well. Maybe at a later date, when I know more, I can apply the fixes there.

Video recording before the PR fix:

node-flicker2.mp4

Video recording after the PR fix:

node-flicker3.mp4

[abrenneke]

Your approach is correct I think! Just a couple thoughts I have!

[abrenneke]

nit: There's a useStableCallback hook you can use which doesn't need a 2nd argument

[codemile]

In this use case, it's not appropriate to use the useStableCallback. As the callbacks defined here are stable and these are not event listener callbacks.

Also, the implementation of useStableCallback is (sorry to say) buggy. As the dependencies of the callback weren't added. As a result, that custom hook is probably introducing some bugs elsewhere in the app. It was on my radar to take a closer look at it.

While on that subject, here is a ref to the React documentation explaining their useEventCallback:

function useEventCallback(fn, dependencies) {
  const ref = useRef(() => {
    throw new Error('Cannot call an event handler while rendering.');
  });

  useEffect(() => {
    ref.current = fn;
  }, [fn, ...dependencies]);

  return useCallback(() => {
    const fn = ref.current;
    return fn();
  }, [ref]);
}

Above, we can see the dependencies is used to trigger useEffect when the callback function mutates. So why are they doing this? Well, it mutates the "ref" instead of triggering prop drilling by any components using the callback. Thus, components using the callback remain stable while the useRef gets updated with the callback.

This is really important in their example, that this pattern can only work with DOM event listeners. As updating of the useRef is lagging behind render updates.

We've used this custom hook in many places, in particular, with callbacks that had dependencies which weren't defined and in cases where the callbacks aren't event listeners. So I feel we need to go through the code and clean up the usages, and as a side effect will probably fix some bugs which we weren't tracking.

FYI, the bug I'm referring to is here in our code useUnrenderedValue.ts:

  useLayoutEffect(() => {
    unrenderedValueRef.current.value = value;
  });

This becomes a 1 time update of the ref with the original callback and that's not correct. It's a very easy fix (not a big deal) and we can address this with another PR later.

[abrenneke]

Interesting! @ZhangYiJiang I think you wrote it what's your take?

[ZhangYiJiang]

This is really important in their example, that this pattern can only work with DOM event listeners.

Yeah this is what useStableCallback is specifically for (it could also be used in useEffect or any sort of side effect). There is in fact a proposal to roll something similar into React core called useEvent although I think that did not make it. Our version could use a better name but this is in fact a feature not a bug

[abrenneke]

was I using it in an effect? my bad

[abrenneke]

I think this looks great now! Thanks for updating it!!