Frontend architecture proposal

[bvaughn]

The frontend's "state" is currently split between several places:

Source	Persisted?	Mutable?	Description
Replay protocol	✓	✘	Original recording data (e.g. screenshots, console logs, network requests)
Apollo/GraphQL	✓	✓	User features/settings, session info, comments on recordings
Redux and React	✘	✓	Transient stuff like focus and current time, derived recording data

Drawbacks

There are a couple of challenges with our current architecture:

• Certain data/behaviors require complex coordination between all three sources.
  • For instance, rendering messages to the Console requires:
    1. Reading the focus region (from Redux).
    2. Asking the backend (Replay protocol) to send us console logs for that region.
    3. Reading log points (from Redux) and then trimming them for the focus region.
    4. Merging both console logs and log points together.
• Requests to the backend (and Apollo) are scattered throughout our codebase, often in a mix of Redux thunks and React component effects. This has a few drawbacks:
  1. One-off calls tend to result in repetition with subtle differences between call sites.
  2. Testing is complicated because...
     • The source of truth is split between three interdependent technologies.
     • It's difficult to mock/stub backend APIs called (deeply) within components.
  3. Fetching data in components leads to a "waterfall" effect.
• Responding to data from the backend is also often scattered throughout the codebase.
  • For instance, handling Console messages is done in a few places:
    1. The ThreadFront instance, which fetches the initial set of messages for a recording, and
    2. The messages Redux action, which refines messages when a new focus region is set

Proposal

This proposal does not aim to address all of the draw backs above, but I think we could simplify things a lot by revisiting the way we manage state.

First, I suggest we re-structure our state as shown in this Figma into three main categories:

• Persisted state:
  • Replay protocol: Persisted, immutable data
  • GraphQL: Persisted, mutable data
• In memory state:
  • Some kind of client-side Replay cache (mutable store? Suspense caches?)
  • Global/shared mutable state
  • Local mutable state
• Derived values
  • Selectors (e.g. custom hooks with useMemo)

Here is a visual outline of what might go where:

I'm still exploring this idea but here are a few constraints I think we should aim for:

• Written with React's Suspense data fetching compatibility in mind (to simplify future adoption of concurrent features).
• Written with testability in mind. (Some basic strategy for how to stub/mock functionality could theoretically be built into such an interface.)
• Typed APIs for frontend actions (e.g. applyFocusRegion(focusRegion: FocusRegion | null): void) and typed event handler API for backend events (e.g. "console-messages" change event when console messages are updated).
  • No direct imports or calls to client, thread, or socket packages.
• Single source of truth for Replay data (like a Suspense cache or possibly a mutable store) and shared selectors (e.g. custom hooks with useMemo) to compute derived values.
  • This could provide a centralized place to add in-memory cache optimizations.
    • For instance, rather than caching the most recent conosle messages for a given focus region, we could cache all of them and merge overlapping regions as new data comes in.

[bvaughn]

We could take this proposal a step further (and address some concerns @markerikson has also raised e.g. the single event handler limitation of socket.js) by limiting all communication between the frontend and the backend to go through this interface.

I'd need to do some research before coming up with a strict proposal, but I imagine this could provide additional benefits:

• Single place to add batching, throttling, or rate limiting (should we choose to) or to implement some form of priority or cancelling/override mechanism.
• Consistent interface for request/response to the backend.
• Single place to subscribe to backend updates and observe changes in state.
• Single place to debug backend communication.

[jcmorrow]

I really like this idea, and I'm happy to help however I can!

[bvaughn]

PRs #6992 and #7026 are an initial exploration of what this new architecture might look like if we ran with some of the ideas mentioned in this thread. That PR is primarily focused on what the React integration should feel like (how would we write new components? how would we write tests?) and doesn't go too far in exploring the "protocol interface" but I think it's a good start.

There are a few open questions or things to follow up on in the PR. I'll list them here too for visibility.

WiredMessages and ValueFronts

The Replay frontend currently massages the backend's Message objects to a similar WiredMessage type, and converts its argumentValues to ValueFronts which it also then calls "load" on during render. This is kind of clunky.

My architecture fork kind of leaves this for now with a TODO comment:
https://github.com/RecordReplay/devtools/blob/0e2ee8091a38aaf1b616648a7ecad633e94d4320/packages/bvaughn-architecture-demo/components/console/MessageRenderer.tsx#L17-L35

I think we could probably move this logic into a Suspense cache. It's just a matter of which one. Should it be something the MessagesCache does?

Protocol interface

Part of the new architecture proposal is that all Replay backend requests should go through some single interface that we'd inject via Context. This interface would give us a single place to add things like caching or priority queues, and using it (and context) in this way makes testing easier (see the PR for some example tests).

The PR implements a super basic version of the interface, but we could do more to flush it out.
https://github.com/RecordReplay/devtools/blob/0e2ee8091a38aaf1b616648a7ecad633e94d4320/packages/bvaughn-architecture-demo/src/ReplayClient.ts#L12-L14

Protocol logger

Building on top of the concept above, Replay devtools could inject some sort of wrapper around the protocol interface to log debug data for Replay engineers (like cache hits/misses)– similar to the Protocol logger. This is just an idea and not a blocker.
https://github.com/RecordReplay/devtools/blob/0e2ee8091a38aaf1b616648a7ecad633e94d4320/packages/bvaughn-architecture-demo/pages/index.tsx#L105-L106

Improved caching

Right now the messages Suspense cache only keeps one chunk of messages in memory at a time. If we need to load a new range of messages, we toss out the old chunk– but (assuming no overflow) we could keep it around and gradually merge them over time so we would never have to re-request the same region again. This is an optimization and not a blocker:
https://github.com/RecordReplay/devtools/blob/0e2ee8091a38aaf1b616648a7ecad633e94d4320/packages/bvaughn-architecture-demo/src/suspense/MessagesCache.ts#L144-L149

Similarly we could allow the points cache to be pre-populated as new info comes in (like paints) so that we could avoid unnecessary time-to-point lookups. (These lookups are fast so this is also just an optimization.)
https://github.com/RecordReplay/devtools/blob/0e2ee8091a38aaf1b616648a7ecad633e94d4320/packages/bvaughn-architecture-demo/src/suspense/PointsCache.ts#L8-L9

[bvaughn]

PR #7099 takes an initial stab at exploring GraphQL integration (with a Suspense cache backing). No GraphQL client is used, just the Fetch API. This PR adds comments and replies to the prototype– and moves pending comments to GraphQL (rather than in memory).

[markerikson]

Pasting in a brief list of some of the things that have concerned me about the codebase as of late:

• ValueFront objects - they're classes, they're stored in Redux, they're mutable and async
  • object previews, parameters, etc
• logpoint logic is horribly complex (thunks on top of thunks)
• console message components are hard to work with
• Mixture between ThreadFront, socket.ts, client/commands - what does each file do? how do they relate to each other? how much of this is app vs "protocol"?
• Passing around cx values so that thunks and the "context middleware" can figure out whether we're still at the same pause point we were before making an async request

[bvaughn]

Agreed!

Regarding ValueFront – I'd like to see us move away from these uncanny value objects that add a little local behavior and instead:

• Use the values returned by GraphQL directly, as-is, so we share the type info and don't have to have a bunch of spread and assign operators to massage the data format before passing it to the components.
• Use a secondary cache (probably Suspense) for related values we need to load asynchronously for display by our React components.

Put another way, rather than having code that does this:

// In one place (that fetches data from GraphQL)
function fetchDatA() {
  // Fetch...
  const clientValue = {
    ...graphQLValue,
    nestedValues: graphQLValue.nestedValues.map(value => new ValueFront(value)),
  };
  return clientValue
}

// In another place (often a React component)
clientValue.nestedValues.forEach(value => value.loadIfNecessary());

We'd have code that does this:

// In one place (that fetches data from GraphQL)
function fetchDatA() {
  // Fetch...
  return graphQLValue;
}

// In another place (a React component, backed by a Suspense cache)
const externalData = loadExternalData(nestedValue);

[jcmorrow]

logpoint logic is horribly complex (thunks on top of thunks)

I think a particularly good example of this is the way we get breakable positions. Here's my diagram trying to understand all of the connected calls (some of which don't even reference each other, you just have to "know" where to look).

And all of those calls ultimately just make one protocol request with one layer of caching in between.

[markerikson]

Tossed out a request for suggestions on tools/techniques to use for a caching layer on Twitter:

https://twitter.com/acemarke/status/1534606214855806977

[bvaughn]

This comment is an example of where our state seems to currently be structured particularly badly (lots of confusing redundancy):

https://github.com/replayio/devtools/issues/7108#issuecomment-1151313917

[markerikson]

I've put together another POC prototype. This one shows use of RTK Query to manage data fetching status (via wrapping our low-level protocol SDK), and some very basic use of plain JS data for displaying stack frames for a given Pause:

#7158

I'd like to expand on this to try doing a few more things related to our current ValueFront usage

[markerikson]

Copying over my comment on the pros and cons of this initial PR:

• RTKQ doesn't have Suspense support yet, because A) we just haven't built it, and B) the current React team (Dan in particular) keeps telling everyone "it's alpha, it may change, don't use this yet": https://twitter.com/dan_abramov/status/1482020154187866125 , https://twitter.com/dan_abramov/status/1482035364743241728
• There is a POC PR to add Suspense support to RTKQ, but the author is still experimenting trying to figure out potential API designs: feat: add useSuspendAll hook & react/suspense example reduxjs/redux-toolkit#2245
• RTKQ testing in general is "mock the network layer using msw and then test connected components with a real store". In this case, we can't use msw, because our protocol client is a wrapper over websockets, not plain HTTP. So, that's kind of an open question.
• There shouldn't be any "race condition" concerns with the cleanup behavior in that getPause endpoint. When the "subscription" reference count for a cache entry goes to 0, RTKQ starts a timer (60 seconds by default). If no other components resume asking for that data before the timer expires, RTKQ will evict the cache entry, and that is when the cacheEntryRemoved promise will resolve. In other words, toggling back and forth between two selected timestamps will just swap the reference counter and each time we'll still have the previous value in cache already.
• Per the "using RTKQ for non-REST data is awkward": I'd sorta disagree with this, because A) this is actually the reason why the queryFn option exists, and B) we'd still have to write those client SDK calls somewhere anyway (and manage the loading state ourselves).

My biggest potential concerns with the overall approach here are:

• While the endpoints themselves are defined outside React, some of the orchestration here is happening in a component (in terms of "fetch A, then B, then C").
• We may need to fetch more related data, and I haven't tried doing that yet
• Testing and websockets
• We've talked about trying to put a lot of the backend interactions into a UI/app-agnostic "protocol" package with a a caching layer. The RTKQ stuff would be firmly in the "app-specific logic" layer instead. So, somewhat at odds with the idea of doing that. (On the flip side, just because this demo uses the @replayio/protocol package to do the fetching doesn't mean a final implementation has to - if the "protocol package" exposes async logic, we could certainly use that here instead.)

[bvaughn]

I think we may be thinking about slightly different aspects RE the Suspense and transition integration. I know RTK doesn't have Suspense support yet. I think my concerns are more that...using an abstraction like RTK, which isn't intended for specifically taking advantage of React APIs like Suspense and transition, might mean that we end up missing out on some of the benefits of those APIs.

RE the "fetch A, then B, then C" angle– this might be an example of what I'm talking about. The React team has investigated different APIs for prefetching (or priming a Suspense cache) for data like this so that – in the worst case, a component suspends while waiting for it, but in most cases it's just already loaded. Maybe this is something RTK can do too. It's less obvious to me because I am not familiar with the RTK abstractions.

You're right that the React team has discouraged people from building on top of those APIs yet. On the other hand, I've been working with those APIs for a year or two now, and have been a part of a lot of the internal discussions about future direction, so I think any investigations we make in that direction are pretty well informed.

As for how we'll eventually decide between the two approaches (assuming we don't end up doing a hybrid of both) I imagine we'll want to consider a few factors:

• How ergonomic is it to write view code? (I suspect both approaches are pretty close here.)
• How well does the approach work with suspense and transitions? (I think both are going to unlock a lot of UX advantages that we really wouldn't want to leave on the table.)
• What does the implementation end up looking like? (How much code? How complex are the abstractions?)
• How easy is it to write robust unit tests (preferably while mocking as little as possible)?

[markerikson]

Yeah, we can discuss the Suspense concerns in another meeting in more detail. What I was kinda trying to say earlier was that as someone who's involved in the ecosystem but not actually part of those internal discussions, the public-facing status is "DANGER HERE BE DRAGONS DO NOT TOUCH!" :) So I think that's where part of our mismatched thoughts are happening - you've been involved in those discussions and are comfortable using those APIs, whereas those of us on the outside are still seeing this as "something that isn't done and may still change", which makes it hard to use or figure out how it might work in libs.

Per the "waterfall" and "pre-fetching" bit: RTKQ requests can be triggered imperatively via dispatch(api.endpoint.getThings.initiate()). In this particular demo, though, the "waterfall" isn't specifically a bunch of things happening in a single sequence like a page load, it's just that the user has to progressively click through and select the next item for each step.

Agreed on the evaluation criteria in general!

[bvaughn]

PR #7235 explores what a new Object Inspector (built with Suspense) could look like.