-
Notifications
You must be signed in to change notification settings - Fork 47k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DevTools] Make Element Inspection Feel Snappy #30555
Conversation
We should be just using the Transition value "inspectedElementID". We could useDeferredValue but we already have an implementation of this ID using a reducer but it would be more idiomatic to useDeferredValue.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This timeout is too long. It makes the UI feels sluggish. Chrome also throttles looping timers aggressively which makes it worse. We also shouldn't rely on the throttling at this level. It doesn't help when you spam the receiving side with thousands of messages to process anyway. Instead we need to implement a form of backpressure to avoid sending so much in the first place.
d096ac5
to
c55b626
Compare
if (!this._scheduledFlush) { | ||
this._scheduledFlush = true; | ||
// $FlowFixMe | ||
if (typeof devtoolsJestTestScheduler === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unfortunate hack. I couldn't figure out how to mock this without rewriting all the tests. In particular all the sync one needs to move to async ones.
That's because React itself also uses queueMicrotask and the tests don't expect that those are mocked as a timer, but they expect that the bridge is mocked as a timer.
Now that we don't have the outer Suspense boundary the inner ones that are used around the "View Source" related buttons show up more prominently because the It also takes 200ms for the same reason. It could be useful for something like source mapping to use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this with React Native, works great. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet
There's two problems. The biggest one is that it turns out that Chrome is throttling looping timers that we're using both while polling and for batching bridge traffic. This means that bridge traffic a lot of the time just slows down to 1 second at a time. No wonder it feels sluggish. The only solution is to not use timers for this. Even when it doesn't like in Firefox the batching into 100ms still feels too sluggish. The fix I use is to batch using a microtask instead so we can still batch multiple commands sent in a single event but we never artificially slow down an interaction. I don't think we've reevaluated this for a long time since this was in the initial commit of DevTools to this repo. If it causes other issues we can follow up on those. We really shouldn't use timers for debouncing and such. In fact, React itself recommends against it because we have a better technique with scheduling in Concurrent Mode. The correct way to implement this in the bridge is using a form of back-pressure where we don't keep sending messages until we get a message back and only send the last one that matters. E.g. when moving the cursor over a the elements tab we shouldn't let the backend one-by-one move the DOM node to each one we have ever passed. We should just move to the last one we're currently hovering over. But this can't be done at the bridge layer since it doesn't know if it's a last-one-wins or imperative operation where each one needs to be sent. It needs to be done higher. I'm not currently seeing any perf problems with this new approach but I'm curious on React Native or some thing. RN might need the back-pressure approach. That can be a follow up if we ever find a test case. Finally, the other problem is that we use a Suspense boundary around the Element Inspection. Suspense boundaries are for things that are expected to take a long time to load. This shows a loading state immediately. To avoid flashing when it ends up being fast, React throttles the reveal to 200ms. This means that we take a minimum of 200ms to show the props. The way to show fast async data in React is using a Transition (either using startTransition or useDeferredValue). This lets the old value remaining in place while we're loading the next one. We already implement this using `inspectedElementID` which is the async one. It would be more idiomatic to implement this with useDeferredValue rather than the reducer we have now but same principle. We were just using the wrong ID in a few places so when it synchronously updated they suspended. So I just made them use the inspectedElementID instead. Then I can simply remove the Suspense boundary. Now the selection updates in the tree view synchronously and the sidebar lags a frame or two but it feels instant. It doesn't flash to white between which is key. DiffTrain build for commit facebook@4ea12a1.
There's two problems. The biggest one is that it turns out that Chrome is throttling looping timers that we're using both while polling and for batching bridge traffic. This means that bridge traffic a lot of the time just slows down to 1 second at a time. No wonder it feels sluggish. The only solution is to not use timers for this. Even when it doesn't like in Firefox the batching into 100ms still feels too sluggish. The fix I use is to batch using a microtask instead so we can still batch multiple commands sent in a single event but we never artificially slow down an interaction. I don't think we've reevaluated this for a long time since this was in the initial commit of DevTools to this repo. If it causes other issues we can follow up on those. We really shouldn't use timers for debouncing and such. In fact, React itself recommends against it because we have a better technique with scheduling in Concurrent Mode. The correct way to implement this in the bridge is using a form of back-pressure where we don't keep sending messages until we get a message back and only send the last one that matters. E.g. when moving the cursor over a the elements tab we shouldn't let the backend one-by-one move the DOM node to each one we have ever passed. We should just move to the last one we're currently hovering over. But this can't be done at the bridge layer since it doesn't know if it's a last-one-wins or imperative operation where each one needs to be sent. It needs to be done higher. I'm not currently seeing any perf problems with this new approach but I'm curious on React Native or some thing. RN might need the back-pressure approach. That can be a follow up if we ever find a test case. Finally, the other problem is that we use a Suspense boundary around the Element Inspection. Suspense boundaries are for things that are expected to take a long time to load. This shows a loading state immediately. To avoid flashing when it ends up being fast, React throttles the reveal to 200ms. This means that we take a minimum of 200ms to show the props. The way to show fast async data in React is using a Transition (either using startTransition or useDeferredValue). This lets the old value remaining in place while we're loading the next one. We already implement this using `inspectedElementID` which is the async one. It would be more idiomatic to implement this with useDeferredValue rather than the reducer we have now but same principle. We were just using the wrong ID in a few places so when it synchronously updated they suspended. So I just made them use the inspectedElementID instead. Then I can simply remove the Suspense boundary. Now the selection updates in the tree view synchronously and the sidebar lags a frame or two but it feels instant. It doesn't flash to white between which is key. DiffTrain build for [4ea12a1](facebook@4ea12a1)
this is awesome |
Same principle as #30555. We shouldn't be throttling the UI to make it feel less snappy. Instead, we should use back-pressure to handle it. Normally the browser handles it automatically with frame aligned events. E.g. if the thread can't keep up with sync updates it doesn't send each event but the next one. E.g. pointermove or resize. However, it is possible that we end up queuing too many events if the frontend can't keep up but the solution to this is the same as mentioned in #30555. I.e. to track the last message and only send after we get a response. I still keep the throttle to persist the selection since that affects the disk usage and doesn't have direct UX effects. The main motivation for this change though is that lodash throttle doesn't rely on timers but Date.now which makes it incompatible with most jest helpers which means I can't write tests against these functions properly.
Full list of changes: * refactor: data source for errors and warnings tracking is now in Store ([hoxyq](https://github.com/hoxyq) in [#31010](#31010)) * fix: consider alternate as a key for componentLogsEntry when inspecting raw fiber instance ([hoxyq](https://github.com/hoxyq) in [#31009](#31009)) * Fix: profiling crashes #30661 #28838 ([EdmondChuiHW](https://github.com/EdmondChuiHW) in [#31024](#31024)) * chore: remove using local storage for persisting console settings on the frontend ([hoxyq](https://github.com/hoxyq) in [#31002](#31002)) * feat: display message if user ended up opening hook script ([hoxyq](https://github.com/hoxyq) in [#31000](#31000)) * feat: expose installHook with settings argument from react-devtools-core/backend ([hoxyq](https://github.com/hoxyq) in [#30987](#30987)) * chore: remove settings manager from react-devtools-core ([hoxyq](https://github.com/hoxyq) in [#30986](#30986)) * feat[react-devtools/extension]: use chrome.storage to persist settings across sessions ([hoxyq](https://github.com/hoxyq) in [#30636](#30636)) * refactor[react-devtools]: propagate settings from global hook object to frontend ([hoxyq](https://github.com/hoxyq) in [#30610](#30610)) * chore[react-devtools]: extract some utils into separate modules to unify implementations ([hoxyq](https://github.com/hoxyq) in [#30597](#30597)) * refactor[react-devtools]: move console patching to global hook ([hoxyq](https://github.com/hoxyq) in [#30596](#30596)) * refactor[react-devtools]: remove browserTheme from ConsolePatchSettings ([hoxyq](https://github.com/hoxyq) in [#30566](#30566)) * feat[react-devtools]: add settings to global hook object ([hoxyq](https://github.com/hoxyq) in [#30564](#30564)) * fix: add Error prefix to Error objects names ([hoxyq](https://github.com/hoxyq) in [#30969](#30969)) * Add enableComponentPerformanceTrack Flag ([sebmarkbage](https://github.com/sebmarkbage) in [#30960](#30960)) * fix[rdt/fiber/renderer.js]: getCurrentFiber can be injected as null ([hoxyq](https://github.com/hoxyq) in [#30968](#30968)) * disable `enableSiblingPrerendering` in experimental channel ([gnoff](https://github.com/gnoff) in [#30952](#30952)) * refactor[react-devtools]: initialize renderer interface early ([hoxyq](https://github.com/hoxyq) in [#30946](#30946)) * Start prerendering Suspense retries immediately ([acdlite](https://github.com/acdlite) in [#30934](#30934)) * refactor[Agent/Store]: Store to send messages only after Agent is initialized ([hoxyq](https://github.com/hoxyq) in [#30945](#30945)) * refactor[RendererInterface]: expose onErrorOrWarning and getComponentStack ([hoxyq](https://github.com/hoxyq) in [#30931](#30931)) * Implement getComponentStack and onErrorOrWarning for replayed Flight logs ([sebmarkbage](https://github.com/sebmarkbage) in [#30930](#30930)) * Use Unicode Atom Symbol instead of Atom Emoji ([sebmarkbage](https://github.com/sebmarkbage) in [#30832](#30832)) * Improve Layering Between Console and Renderer ([sebmarkbage](https://github.com/sebmarkbage) in [#30925](#30925)) * Add Map for Server Component Logs ([sebmarkbage](https://github.com/sebmarkbage) in [#30905](#30905)) * Delete fiberToFiberInstanceMap ([sebmarkbage](https://github.com/sebmarkbage) in [#30900](#30900)) * Add Flight Renderer ([sebmarkbage](https://github.com/sebmarkbage) in [#30906](#30906)) * Refactor Error / Warning Count Tracking ([sebmarkbage](https://github.com/sebmarkbage) in [#30899](#30899)) * [flow] Upgrade Flow to 0.245.2 ([SamChou19815](https://github.com/SamChou19815) in [#30919](#30919)) * Separate RDT Fusebox into single-panel entry points ([huntie](https://github.com/huntie) in [#30708](#30708)) * Build Updater List from the Commit instead of Map ([sebmarkbage](https://github.com/sebmarkbage) in [#30897](#30897)) * Simplify Context Change Tracking in Profiler ([sebmarkbage](https://github.com/sebmarkbage) in [#30896](#30896)) * Remove use of .alternate in root and recordProfilingDurations ([sebmarkbage](https://github.com/sebmarkbage) in [#30895](#30895)) * Handle reordered contexts in Profiler ([sebmarkbage](https://github.com/sebmarkbage) in [#30887](#30887)) * Refactor Forcing Fallback / Error of Suspense / Error Boundaries ([sebmarkbage](https://github.com/sebmarkbage) in [#30870](#30870)) * Avoid getFiberIDUnsafe in debug() Helper ([sebmarkbage](https://github.com/sebmarkbage) in [#30878](#30878)) * Include some Filtered Fiber Instances ([sebmarkbage](https://github.com/sebmarkbage) in [#30865](#30865)) * Track root instances in a root Map ([sebmarkbage](https://github.com/sebmarkbage) in [#30875](#30875)) * Track all public HostInstances in a Map ([sebmarkbage](https://github.com/sebmarkbage) in [#30831](#30831)) * Support VirtualInstances in findAllCurrentHostInstances ([sebmarkbage](https://github.com/sebmarkbage) in [#30853](#30853)) * Add Filtering of Environment Names ([sebmarkbage](https://github.com/sebmarkbage) in [#30850](#30850)) * Support secondary environment name when it changes ([sebmarkbage](https://github.com/sebmarkbage) in [#30842](#30842)) * Increase max payload for websocket in standalone app ([runeb](https://github.com/runeb) in [#30848](#30848)) * Filter Server Components ([sebmarkbage](https://github.com/sebmarkbage) in [#30839](#30839)) * Track virtual instances on the tracked path for selections ([sebmarkbage](https://github.com/sebmarkbage) in [#30802](#30802)) * Remove displayName from inspected data ([sebmarkbage](https://github.com/sebmarkbage) in [#30841](#30841)) * chore[react-devtools/hook]: remove unused native values ([hoxyq](https://github.com/hoxyq) in [#30827](#30827)) * chore[react-devtools/extensions]: remove unused storage permission ([hoxyq](https://github.com/hoxyq) in [#30826](#30826)) * fix[react-devtools/extensions]: fixed tabs API calls and displaying restricted access popup ([hoxyq](https://github.com/hoxyq) in [#30825](#30825)) * feat[react-devtools]: support Manifest v3 for Firefox extension ([hoxyq](https://github.com/hoxyq) in [#30824](#30824)) * Reconcile Fibers Against Previous Children Instances ([sebmarkbage](https://github.com/sebmarkbage) in [#30822](#30822)) * Remove findCurrentFiberUsingSlowPathByFiberInstance ([sebmarkbage](https://github.com/sebmarkbage) in [#30818](#30818)) * Track Tree Base Duration of Virtual Instances ([sebmarkbage](https://github.com/sebmarkbage) in [#30817](#30817)) * Use Owner Stacks to Implement View Source of a Server Component ([sebmarkbage](https://github.com/sebmarkbage) in [#30798](#30798)) * Make function inspection instant ([sebmarkbage](https://github.com/sebmarkbage) in [#30786](#30786)) * Make Functions Clickable to Jump to Definition ([sebmarkbage](https://github.com/sebmarkbage) in [#30769](#30769)) * Support REACT_LEGACY_ELEMENT_TYPE for formatting JSX ([sebmarkbage](https://github.com/sebmarkbage) in [#30779](#30779)) * Find owners from the parent path that matches the Fiber or ReactComponentInfo ([sebmarkbage](https://github.com/sebmarkbage) in [#30717](#30717)) * [Flight/DevTools] Pass the Server Component's "key" as Part of the ReactComponentInfo ([sebmarkbage](https://github.com/sebmarkbage) in [#30703](#30703)) * Hide props section if it is null ([sebmarkbage](https://github.com/sebmarkbage) in [#30696](#30696)) * Support Server Components in Tree ([sebmarkbage](https://github.com/sebmarkbage) in [#30684](#30684)) * fix[react-devtools/InspectedElement]: fixed border stylings when some of the panels are not rendered ([hoxyq](https://github.com/hoxyq) in [#30676](#30676)) * Compute new reordered child set from the instance tree ([sebmarkbage](https://github.com/sebmarkbage) in [#30668](#30668)) * Unmount instance by walking the instance tree instead of the fiber tree ([sebmarkbage](https://github.com/sebmarkbage) in [#30665](#30665)) * Further Refactoring of Unmounts ([sebmarkbage](https://github.com/sebmarkbage) in [#30658](#30658)) * Remove lodash.throttle ([sebmarkbage](https://github.com/sebmarkbage) in [#30657](#30657)) * Unmount by walking previous nodes no longer in the new tree ([sebmarkbage](https://github.com/sebmarkbage) in [#30644](#30644)) * Build up DevTools Instance Shadow Tree ([sebmarkbage](https://github.com/sebmarkbage) in [#30625](#30625)) * chore[packages/react-devtools]: remove unused index.js ([hoxyq](https://github.com/hoxyq) in [#30579](#30579)) * Track DOM nodes to Fiber map for HostHoistable Resources ([sebmarkbage](https://github.com/sebmarkbage) in [#30590](#30590)) * Rename mountFiberRecursively/updateFiberRecursively ([sebmarkbage](https://github.com/sebmarkbage) in [#30586](#30586)) * Allow Highlighting/Inspect HostSingletons/Hoistables and Resources ([sebmarkbage](https://github.com/sebmarkbage) in [#30584](#30584)) * chore[react-devtools]: add global for native and use it to fork backend implementation ([hoxyq](https://github.com/hoxyq) in [#30533](#30533)) * Enable pointEvents while scrolling ([sebmarkbage](https://github.com/sebmarkbage) in [#30560](#30560)) * Make Element Inspection Feel Snappy ([sebmarkbage](https://github.com/sebmarkbage) in [#30555](#30555)) * Track the parent DevToolsInstance while mounting a tree ([sebmarkbage](https://github.com/sebmarkbage) in [#30542](#30542)) * Add DevToolsInstance to Store Stateful Information ([sebmarkbage](https://github.com/sebmarkbage) in [#30517](#30517)) * Implement "best renderer" by taking the inner most matched node ([sebmarkbage](https://github.com/sebmarkbage) in [#30494](#30494)) * Rename NativeElement to HostInstance in the Bridge ([sebmarkbage](https://github.com/sebmarkbage) in [#30491](#30491)) * Rename Fiber to Element in the Bridge Protocol and RendererInterface ([sebmarkbage](https://github.com/sebmarkbage) in [#30490](#30490)) * Stop filtering owner stacks ([sebmarkbage](https://github.com/sebmarkbage) in [#30438](#30438)) * [Fiber] Call life-cycles with a react-stack-bottom-frame stack frame ([sebmarkbage](https://github.com/sebmarkbage) in [#30429](#30429)) * [Flight] Prefix owner stacks added to the console.log with the current stack ([sebmarkbage](https://github.com/sebmarkbage) in [#30427](#30427)) * [BE] switch to hermes parser for prettier ([kassens](https://github.com/kassens) in [#30421](#30421)) * Implement Owner Stacks ([sebmarkbage](https://github.com/sebmarkbage) in [#30417](#30417)) * [BE] upgrade prettier to 3.3.3 ([kassens](https://github.com/kassens) in [#30420](#30420)) * [ci] Add yarn_test_build job to gh actions * [Fizz] Refactor Component Stack Nodes ([sebmarkbage](https://github.com/sebmarkbage) in [#30298](#30298)) * Print component stacks as error objects to get source mapping ([sebmarkbage](https://github.com/sebmarkbage) in [#30289](#30289)) * Upgrade flow to 0.235.0 ([kassens](https://github.com/kassens) in [#30118](#30118)) * fix: path handling in react devtools ([Jack-Works](https://github.com/Jack-Works) in [#29199](#29199))
There's two problems. The biggest one is that it turns out that Chrome is throttling looping timers that we're using both while polling and for batching bridge traffic. This means that bridge traffic a lot of the time just slows down to 1 second at a time. No wonder it feels sluggish. The only solution is to not use timers for this.
Even when it doesn't like in Firefox the batching into 100ms still feels too sluggish.
The fix I use is to batch using a microtask instead so we can still batch multiple commands sent in a single event but we never artificially slow down an interaction.
I don't think we've reevaluated this for a long time since this was in the initial commit of DevTools to this repo. If it causes other issues we can follow up on those.
We really shouldn't use timers for debouncing and such. In fact, React itself recommends against it because we have a better technique with scheduling in Concurrent Mode. The correct way to implement this in the bridge is using a form of back-pressure where we don't keep sending messages until we get a message back and only send the last one that matters. E.g. when moving the cursor over a the elements tab we shouldn't let the backend one-by-one move the DOM node to each one we have ever passed. We should just move to the last one we're currently hovering over. But this can't be done at the bridge layer since it doesn't know if it's a last-one-wins or imperative operation where each one needs to be sent. It needs to be done higher. I'm not currently seeing any perf problems with this new approach but I'm curious on React Native or some thing. RN might need the back-pressure approach. That can be a follow up if we ever find a test case.
Finally, the other problem is that we use a Suspense boundary around the Element Inspection. Suspense boundaries are for things that are expected to take a long time to load. This shows a loading state immediately. To avoid flashing when it ends up being fast, React throttles the reveal to 200ms. This means that we take a minimum of 200ms to show the props. The way to show fast async data in React is using a Transition (either using startTransition or useDeferredValue). This lets the old value remaining in place while we're loading the next one.
We already implement this using
inspectedElementID
which is the async one. It would be more idiomatic to implement this with useDeferredValue rather than the reducer we have now but same principle. We were just using the wrong ID in a few places so when it synchronously updated they suspended. So I just made them use the inspectedElementID instead.Then I can simply remove the Suspense boundary. Now the selection updates in the tree view synchronously and the sidebar lags a frame or two but it feels instant. It doesn't flash to white between which is key.