Proposal: Track app settling

[jtran]

I don't know if this will work, but here is a way that I thought it could be done.

1. Always await or return a Promise.
2. If you can't and need to fire something off, like in a click-handler, instead of using void promise to ignore it, call trackAsyncWork(promise).
3. Something else, like a test, can install a callback that gets called when all promises settle, using addSettleCallback().

The lint already checks that we await, return, or use void for all promises. So this requires that we don't use void through diligence, which is not great.

Would this help? Would this not work? Discuss. 🙏

let appPromises = []
let onSettleCallbacks = []

// Native Promises don't have a way to synchronously detect if they're settled.
class TrackedPromise {
  constructor(promise: Promise) {
    this.settled = false;
    this.promise = promise.then(() => {
      this.settled = true;
      debounce(attemptCleanUp, 50);
    }, () => {
      this.settled = true;
      debounce(attemptCleanUp, 50);
    })
  }
}

export function trackAsyncWork<T>(promise: Promise<T>, onSettle?: () => void) {
  // I considered using a WeakSet or a WeakRef for each.
  // But I think they both have problems.
  appPromises.push(new TrackedPromise(promise));
  if (onSettle) {
    onSettleCallbacks.push(onSettle);
  }
}

export function addSettleCallback(onSettle: () => void) {
  // TODO: Use a Promise interface instead.
  // TODO: Check if it's already settled.
  onSettleCallbacks.push(onSettle);
}

export function isAppSettled(): boolean {
  return appPromises.every((p) => p.settled);
}

function attemptCleanUp() {
  if (appPromises.length === 0) {
    return;
  }
  // Garbage collect.
  const unsettled = appPromises.filter((p) => !p.settled);
  appPromises = unsettled;
  // Transition to settled.  We could move this into TrackedPromise.
  // It's a trade-off between reducing latency and reducing overhead.
  if (unsettled.length === 0) {
    for (const cb of onSettleCallbacks) {
      cb();
    }
    onSettleCallbacks = [];
  }
}

[lf94]

Motivation?

[jtran]

Motivation 1: Reduce race conditions in tests

Instead of every assertion having retry logic with a timeout:

1. Click in the UI
2. Wait for all settled. If timeout expires, fail the test.
3. Assert the expected result, without any retries or timeout
4. Do the next click or UI interaction

You may be wondering, how is this better if it simply moves the timeout? In the old approach, if there's lots of async work like writing a file to disk, the assertion may pass and move on to the next step (4) before the file was written.

This proposal, on the other hand, essentially creates a synchronization point that waits until all async work is finished before moving on. This means that there are fewer race conditions in the testing code.

Motivation 2: Reduce race conditions in the app

We could start using a similar approach in the app itself, not just tests.

For example, when transitioning from one state to another, maybe all async work is awaited and a UI spinner is shown before going to the next state. I don't think a blanket approach can be used since we don't necessarily want to introduce loading states all over the app. But this proposal would make it possible for when we would want it.

Questions

Maybe there are certain classes of async things that we wouldn't want to wait for, like engine calls. We might have to make an exception for them and not track them (or track them separately).

[jtran]

The conclusion from last week's meeting was that this is a good idea to make tests more resilient, but not to be used for app logic. For that, we want to lean-in to "data down, ~actions~ events up", and add state machines to handle the events and protect the resources. See https://github.com/KittyCAD/react-xstate-demo for an example of using XState in this way.