ENGWORK-5595: Make MVC compatible with React 18

[joshwilsonvu]

Tearing

The current implementation of MVC is susceptible to a problem called "tearing" under React 18. React 18 doesn't always run a render (calling components) in one synchronous pass—it can break the work up into chunks to allow the browser to be more responsive ("time-slicing").

The trouble comes in when a component reads from a store outside React during render. If a render is split into two chunks of work, and the store changes in between, then you'll see inconsistencies onscreen. Different components "saw" different store values.

I wrote a blog post a while back explaining tearing in more depth. (We never published it bc it recommended a different solution.)

useSyncExternalStore

React's solution to this is useSyncExternalStore. It takes a subscribe function and getSnapshot function—subscribe informs React when a store value has changed, and getSnapshot lets React check at any time what the current store value is. React is optimistic and will assume it's safe to time-slice renders, but will sanity-check that getSnapshot() hasn't changed; if it has, it'll retry the render synchronously to guarantee that the store value won't change. (Go back and read that "tearing" link if this doesn't click, there's pictures there.)

Why do we need this? It works fine already!

We've generally avoided this problem so far by limiting our usage of React 18-specific, Concurrent features like <Suspense>, useTransition(), useDeferredValue(), and to some extent React.lazy(). (There's also a super handy use() hook in React 19 that can be backported to React 18.) If you don't use those features, React won't use time-slicing.

This PR makes it safe to use Concurrent features by implementing MVC APIs (primarily ApplicationView) with useSyncExternalStore. It heavily leans on MobX's code, which uses the same View() wrapper strategy and is battle-tested.

As a bonus, it takes advantage of React 18's auto-batching to support using MVC state for controlled inputs! All updates in an event handler (e.g. onChange) will be batched and run synchronously, and iirc all other updates will be batched and run at the end of the microtask.

Benefits of Concurrent features

Declarative data fetching

With Concurrent features, we could use controllers to back declarative data fetching. This truly treats React as a view layer for the business logic in our controllers. If something is not ready, we don't have to deal with a bunch of isLoading flags—we can just add a fallback, and write our components as if everything is ready.

class Controller extends ApplicationController {
  initialize(props) {
    this.state.dataPromise = fetch(`https://some.api.com/${props.dataUrl}`)
  }
}
function Component() {
  const controller = useController(Controller);
  const data = use(controller.state.dataPromise); // <- suspends until data is ready
  return <p>{data}</p>
}
// <Suspense fallback={<Spinner />}>
//   <Component>
// </Suspense>

This especially helps with cases where you have multiple spinners that "popcorn" into view at different times. Suspense can coordinate loading states so that all content is revealed at once, or whatever granularity you want, without the individual components knowing about it.

Code-splitting with React.lazy

We could reduce the size of our massive JS bundle with code splitting, by lazy-loading components when they're rendered.

const Component = React.lazy(() => import('path/to/some/Component'))

// <Suspense fallback={<Spinner />}>
//   <Component />
// </Suspense>

Why can't we do this already? When React renders <Component />, it knows the code for that component isn't loaded yet and kicks it off. It also abandons the current render. When the code loads, it restarts the render. useSyncExternalStore ensures that any changes to state in the meantime are caught and don't cause issues with the rendered output.

Fancier React hooks

This lets us safely use the hooks useTransition and useDeferredValue. They're useful in niche cases. There are some caveats.

Reduced risk of library upgrades

Even if we don't use the features above, they're fair game for the npm libraries we use, and typically show up more often in library code than application code. There's nothing preventing a patch or minor upgrade from opting React into concurrent rendering (for its subtree) and causing odd behavior or bugs that useSyncExternalStore protects us from.

Misc

• With Jest, I couldn't use structuredClone despite good browser support; it was easier to switch to Vitest than to fix it. Switched back to Jest because structuredClone broke aha-app Jest tests—I'd rather catch Jest issues early.
• This approach locks us out of using the React Compiler because we are still accessing mutable state objects during render, and the Compiler assumes immutable state.
• Updated the tsconfig.json and a few other packages too, just maintenance since this repo doesn't get touched often.

---

• ENGWORK-5595: Minor changes to get tests passing; add concurrency-focused tests.
• ENGWORK-5595: Switch to Vitest (easier than making structuredClone work with Jest) and modernize TSConfig.
• ENGWORK-5595: Make ApplicationController concurrency-safe and type-safe with same useLifecycleBoundObject trick.
• ENGWORK-5595: Remove View class component handling, extract GC stuff into useLifecycleBoundObject, docs and types.
• ENGWORK-5595: Simplify randomId to match aha-app.
• ENGWORK-5595: Updated core logic in ApplicationView to be React 18 compatible. Started making ApplicationController compatible.

[gregplaysguitar]

tsdx is awesome, I'd be happy to adopt that

[joshwilsonvu]

this PR is getting pretty large, I'm hesitant to add more changes anymore 😅

[gregplaysguitar]

I found at least one instance in our code which breaks here, because there might be no controller of the provided class. We probaably need to keep the existing behaviour.

[joshwilsonvu]

tried removing this, but it breaks the types—it would force me to return TController | undefined. Would you be open to a separate "safe" hook like useControllerIfPresent that can return undefined (or null) instead of throwing an error? I definitely don't want to make the main useController() nullable as that'll break tons of code, but I also don't want to cast it and lie about the types 😓

[gregplaysguitar]

I'm not sure we should encourage accessing the controller's props externally. I know we do it, but I think it'd be better as you suggest to just pass props to the subcomponents. controller.props is more intended to be used within the controller class itself.

The controller's props are all passed on to the wrapped component itself, so I think rather than providing this hook, we should just suggest grabbing them from there if needed.

[joshwilsonvu]

I'm 100% with you. I'm not trying to encourage this--it's just here to address cases where we already do use controller.props in nested components without larger refactors.

Because controller.props now has to be updated in an effect, after a render, it causes additional renders and temporary tearing to access them in nested components. The hook is just plain context so you can avoid that issue with a quick refactor. Using context to avoid prop-drilling is what people seem to want anyway in the cases I've seen, even if prop drilling is probably better.