Skip to content

Conversation

@markerikson
Copy link
Collaborator

@markerikson markerikson commented Apr 29, 2022

This PR:

  • Adds a dependency on the @redux-devtools/app package, which has the prebuilt Redux DevTools UI components and logic
  • Adds a new "Redux" panel to the toolbox pane
  • Renders the RDT UI
  • Actually manages to feed in actual Redux actions received via annotations from the backend
  • Actually manages to use timeline updates to reset the list of actions and feed in just the ones that were dispatched up to that point in time
  • Uses next-global-css to work around Next throwing errors due to the RDT <Editor> component importing CSS from within node_modules

Status

Totally a draft POC and absolutely not ready to ship in any way :)

At the moment, looks like a bunch of the supporting code I copy-pasted from the RDT "extension app" codebase is not compiling because it's missing other pieces. Not surprising! I will fix that stuff up on Monday so this at least builds clean.

Things that this needs for minimum productionization:

  • Consideration of how we actually want to handle annotations between React and Redux integration
  • The Redux panel should likely only import the RDT UI (and it should be lazy-loaded, too) if we actually have Redux-related annotations. (Possibly the same for the React panel later?). There's a bunch of bundle size here, and we shouldn't pay that if it's not relevant.
  • For that matter, showing the "Redux" panel should also be an experimental setting
  • The current "tie displayed actions to the timeline progress" technique is hacky. But it does seem to work!
  • The RDT UI theme should be controlled by Replay's own theme selection
  • The RDT logic has a bunch of "feature" settings. We should probably turn off a bunch of options, at least for now. (Like, "skip" doesn't make sense atm.)
  • Probably need to correctly handle "instances" - there can be many "stores" connected to the DevTools in one page
  • Should test this against other frameworks that use the RDT, like Zustand, Jotai, and NgRx
  • Should test this with some really large or frequent actions. (Like, say.... Replay itself! :) )

This is also entirely dependent on the FF content script changes sitting over in replayio/gecko-dev#831 - you've got to use an FF build with those to make the recording, which also means you can't even try this PR out because our prod backend doesn't recognize this FF build! We may need to just go merge in those changes and release a new FF build first to unblock the chicken/egg issue here.

Preview

Since you can't actually see or try this out atm, here's a Loom showing it off:

https://www.loom.com/share/738c236b13cf41fb902335d30c085edb

@vercel
Copy link

vercel bot commented Apr 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
devtools ✅ Ready (Inspect) Visit Preview May 4, 2022 at 3:43PM (UTC)

@markerikson markerikson marked this pull request as draft April 29, 2022 20:33
@markerikson markerikson force-pushed the feature/6583-redux-devtools-ui branch from c42fd07 to 194b4ab Compare April 29, 2022 21:09
Previously, all annotations were fetched in `actions/reactDevTools`.
We can only have one `ThreadFront.getAnnotations()` call, as
`socket.ts` throws errors if you try to add another subscription.

That logic was kicked off from `devtools.ts` as part of the initial
setup sequence. But, that meant that annotations were coming in
even before the rest of the React component tree mounts.

Our React annotations are kept in the Redux store (hah!), but since
Redux actions data can be _very_ large, we don't want to keep those
in our own app's Redux store. The simplest option is adding a
context provider component whose sole job is to keep the Redux
annotations in state.

To make things more complicated, the entire "DevTools" panel will
unmount if you switch over to the "Viewer" tab. So, we can't just
store Redux annotations down low in the component tree.

After discussion with Brian Vaughn, I've addressed these issues
by refactoring how we handle annotations:

- There is now a `<ReduxAnnotationsProvider>` component that is
  rendered in the `<Body>` component and wrapped around both panels
- That component is responsible for running the `getAnnotations()`
  subscription request. Since React's "Strict Effects" can cause
  effects to run twice in dev, we have to use a ref flag to make
  sure it only runs once.
- It still dispatches all the React-specific annotations to the
  Redux store, same as before.
- But, Redux-specific annotations are pre-processed to be in the
  form that we need, added to its own state, and passed down via
  a React context provider.

Additionally, I've done some "productionization" of how the Redux
DevTools tab gets loaded:

- `<SecondaryToolbox>` reads the annotations list and only show
  the "Redux" tab if there are Redux annotations available
- The `<ReduxDevToolsPanel>` component lazy-imports the actual
  `@redux-devtools/app` lib, so that we don't pay that bundle cost
  up front
- The "reload actions into the DevTools store" code uses `batch()`
  to both minimize re-renders and avoid a data inconsistency
  crash that I was seeing
return async dispatch => {
dispatch({ type: "set_protocol_fail" });
};
export function setProtocolCheckFailed(): SetProtocolCheckFailedAction {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 This action creator really didn't need to be a thunk anyway, since it's just dispatching a single basic action.

@markerikson
Copy link
Collaborator Author

Status update:

  • We've published a Firefox build that should capture Redux data. Unfortunately, it doesn't seem to be doing that :( We're going to have to investigate further.
  • I've updated this PR to do a few of the "productionization" items:
    • The "Redux" tab only shows up if there are Redux annotations loaded into the client
    • The @redux-devtools/app package is lazy-imported on demand when you switch over to that tab for the first time
  • I've run this against a Jotai CodeSandbox, and confirmed that A) this does work with other non-Redux packages, and B) it does work okay with multiple "instances" in one page:

First instance - a single Jotai "count" atom:

image

Second instance - a "combined atoms" view:

image

@markerikson markerikson marked this pull request as ready for review May 3, 2022 17:23
@markerikson
Copy link
Collaborator Author

I've updated this to keep the RDT theme in sync with the Replay app theme. Also tried passing down some flags to mark all of the RDT "features" as disabled, in the hopes that its UI would turn off buttons like "Jump" and "Skip", but doesn't seem to be doing what I want.

That said, I think this is basically working at a v1 level at this point.

Caveats:

  • I have not tried it with a larger-scale recording with many actions, ie, a recording of Replay itself
  • We don't yet know why the latest browser build does not appear to be capturing Redux actions as annotations properly, and this PR is kinda useless until we get that figured out

Copy link
Contributor

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jasonLaster jasonLaster merged commit ac248bd into master May 4, 2022
@jcmorrow jcmorrow deleted the feature/6583-redux-devtools-ui branch June 8, 2022 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants