Updating Monaco broke Fastly Fiddle: here's how I solved it with useCallback in React

My colleague Dora and I recently updated Fastly Fiddle's dependencies, and we suddenly found that user input in our code editor was erratic and unusably slow. We fixed it by moving some state from the component into a local variable, persisted across renders thanks to useCallback.

This is what we saw in the UI:

Popping open the network panel, we can see a request to the server for every keystroke, 1 second after the key was pressed:

Ouch. Our debounce is clearly broken - and not only that but we're also supposed to be cancelling save requests that are already in progress if a new keystroke is made, and that clearly isn't working either.

It turns out that the bottom line is, if you read state in a function that's been memoized, you will get an out of date version of that state.

In our case, Monaco, the code editor component, was memoizing a function, invoking an outdated version of it, and as a result we were never able to tell that a save operation was already in progress, so we didn't cancel the existing operation, and cued up a new one.

The problematic code

Many performance problems and behavioural bugs in React applications seem to stem from a too-hazy understanding of what causes a component to re-render, what happens when a component re-renders, and in what context a function is being invoked. Fortunately I am friends with Ivan Akulov who is basically web performance in human form, and he helped me figure this out.

In Fastly Fiddle, we have a state variable saveOperation which is read and updated by a saveHandler function, and which is also passed down into child components:

const [saveOperation, setSaveOperation] = useState();

const saveHandler = async (userInput) => {
  if (saveOperation) saveOperation.abort();
  const debounceDelay = abortableWait(DEBOUNCE_DELAY);
  setSaveOperation(debounceDelay);
  await debounceDelay;
  ...
  setSaveOperation(null);
}

render(<Main onSave={saveHandler} />)

We're trying to wait for an idle period before saving the user's work (a 'debounce'). Every time we see new keystrokes, we abort the pending save - or any in-flight API request that is already in progress.

We're doing this using an abortable promise pattern - there's probably a more idiomatic way to do this in React but this seemed to work pretty well. saveHandler is going to get redefined every time the component renders, but that should be fine - saveOperation is component state, which will persist across renders. Right?

Memoization problem

However, there's a risk - if anything further down the component tree memoizes the reference to saveHandler, we might end up calling an out of date version of the function which has captured an out of date version of saveOperation.

Turns out, this is exactly what's happening. After our dependency update, we discovered that when keystrokes were entered into a code editor component powered by Monaco, the resulting invocation of saveHandler found saveOperation to be undefined every time.

The <Main> component eventually passes the saveHandler function to Monaco like this:

<MonacoEditor
  theme="fiddle"
  key={props.codeContext}
  options={monacoOptions}
  value={props.value}
  onChange={props.saveHandler}
/>

The new version of Monaco then memoizes the onChange callback, which means that when keystrokes are entered into the editor, the saveHandler that gets fired is one that doesn't have the latest value of saveOperation, so we don't know there's a save already in progress, so we don't cancel it, and all hell breaks loose.

Progress state is an antipattern?

So what's the solution? I could investigate why Monaco is memoizing the handler, and maybe try and convince it to not do that, but this problem highlights that using useState to store saveOperation is maybe not the best idea in the first place. Apart from making the function dependent on data in the parent context, using state also means we re-render the component every time we update it. Since saveOperation doesn't affect the component's render output, that doesn't seem right.

I've decided to call this kind of data "progress state", since it effectively carries data forward from one invocation of a function to the next, to allow a subsequent invocation to adjust its behaviour based on knowing where we left off in the previous one.

useState is the most obvious way to do this, but it's not a good one. The React-y way to do this is actually useRef:

Solution 1: useRef

const saveOperation = useRef(null);

const saveHandler = async (userInput) => {
  if (saveOperation.current) saveOperation.current.abort();
  const debounceDelay = abortableWait(DEBOUNCE_DELAY);
  saveOperation.current = debounceDelay;
  await debounceDelay;
  ...
  saveOperation.current = null;
}

Updating saveOperation now won't trigger a render, and this will work even if saveHandler gets memoized.

The reason this works is the same reason useRef variables always have that weird .current thingy hanging off them. The variable created by useRef is immutable, so capturing it into a function is fine, because we know it cannot be reassigned: the value of saveOperation (which is an object) will never change. However, the properties of that object can change, and a function that has captured a reference to the object will see those updated properties. That's why refs have a .current property.

Solution 2: useCallback closures

React's useCallback hook creates a reference to a function and then reuses it in subsequent renders of the component, rather than redefining the function every time. We figured, you could use this with an Immediately Invoked Function Expression (IIFE) to capture a value that persists between renders:

const saveHandler = useCallback((() => {
  let saveOperation;
  return async (userInput) => {
    if (saveOperation) saveOperation.abort();
    saveOperation = abortableWait(DEBOUNCE_DELAY);
    await saveOperation;
    ...
    saveOperation = null;
  };
})(), []);

Now, when the component is first rendered, React will execute the IIFE, and assign the resulting function to saveHandler. The returned function has access to saveOperation which is trapped in a closure created by the IIFE.

I like this solution because:

1. it leans more on fundamentals of JavaScript rather than relying on the framework to solve the problem
2. it creates a tighter scope for my data (saveOperation is not accessible outside of saveHandler)
3. It's the simplest use of saveOperation - no need to use a setter function, and no need to use a .current property.

However, React's docs on useCallback have this caveat:

In the future, React may add more features that take advantage of throwing away the cache — for example, if React adds built-in support for virtualized lists in the future, it would make sense to throw away the cache for items that scroll out of the virtualized table viewport. This should match your expectations if you rely on useCallback as a performance optimization.

So in future, my callback might get redefined more often... but for a use case like debouncing, it seems like that would still be fine most of the time.

Conclusion

There are some nice lessons in this debugging exercise: Don't overuse useState. Learn to love useRef. And it's nice that React has a solution to this and that we can also solve it using JS language primitives.

Fine, go ahead and flame me for doing React wrong. I don't care, React is weird anyway.