feat(devtools): Register event subscriptions on-demand#27591
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (538 lines, 9 files), I've queued these reviewers:
How this works
|
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
| @@ -341,8 +381,11 @@ export class DataVisualizerGraph | |||
| * | |||
| * A visual representation can be requested via {@link VisualizerNode.render}. | |||
| * | |||
| * Additionally, whenever the associated `ISharedObject` is updated (i.e. whenever its "op" event is emitted), | |||
| * an updated visual tree will be emitted via this object's {@link SharedObjectListenerEvents | "update" event}. | |||
| * Additionally, while at least one consumer is {@link VisualizerNode.subscribe | subscribed}, whenever the associated | |||
There was a problem hiding this comment.
NIT: Should we repalce "whenever" to "when subscribed" mirroring the class-level JSDOC from above?
|
NIT: While renaming existing types to |
| * Initialize data visualization monitoring immediately to ensure event listeners are set up | ||
| * and console logs appear even when devtools UI is not open. | ||
| */ | ||
| private async initializeDataVisualizationMonitoring(): Promise<void> { |
There was a problem hiding this comment.
Removing the startup monitoring here — together with moving the op listener out of VisualizerNode's constructor and into subscribe() — means the client now only attaches an op listener (and therefore only broadcasts DataVisualization updates) for objects a consumer has explicitly subscribed to via GetDataVisualization.
I think that regresses the sidebar "container has changes" indicator. The only code path that emits reason: DataChanged is dataUpdateHandler, which fires off the DataVisualizerGraph "update" event — and after this change that event only fires while a VisualizerNode has an active subscription. But Menu.tsx drives the indicator purely from inbound DataVisualization messages with reason === DataChanged, while only ever sending GetContainerState, never GetDataVisualization:
FluidFramework/packages/tools/devtools/devtools-view/src/components/Menu.tsx
Lines 765 to 788 in a072ce2
Since FluidHandleView (the detail panel) is the only sender of GetDataVisualization, no op listener exists for a container's objects unless the user has that specific object's detail view open — so the "has changes" indicator will no longer light up for anything the user hasn't drilled into. Previously this method rendered every root "to set up event listeners on all shared objects" at startup, so the indicator fired regardless.
The new DataVisualizerGraph test "Broadcasts updates while subscribed and stops after unsubscribe" actually captures the mechanism: an op emitted before subscribe() yields updateCount === 0. For the objects the menu cares about (which are never subscribed), that zero-broadcast path is exactly the regression.
Is this intended / handled in a follow-up? I didn't see the indicator called out in the compatibility matrix. If it's meant to keep working, it likely needs an always-on signal independent of the detail-panel subscription — e.g. the menu subscribing per container, or a lightweight "changed" notification decoupled from full visualization.
There was a problem hiding this comment.
Interesting. I'd forgotten about that indicator - thanks for the callout. I wish it was easier to write integration tests for these scenarios 😟
So, I think this is a regression we probably want to take, though it is definitely worth considering how we should update the change indicator in response. My main reasons for this are:
-
Performance - the fact that apps using devtools immediately pay a non-trivial cost in listening for DDS changes and broadcasting messages on the window (even when no one is listening) is a deterrent for using them at all. We were forced to disable the devtools (by default) in our text demo because the performance hit was so substantial.
-
This issue already exists for any nested DDSs - in our visualizer graph (currently), we eagerly bind listeners and post updates upon devtools initialization, but only for the root DDSs declared in the input set. Any other DDSs referenced by handle already don't publish updates until the view (extension) requests them.
Consider the following setup: devtools is configured with a single SharedDirectory which contains a single child item: a handle to a SharedTree. The corresponding application exclusively uses the SharedTree, and never directly changes its directory contents. Our change indicator would already never fire in this case.
Question for you: was the change indicator a requirement for anything? Accessibility maybe? Or was it just a nice-to-have UX improvement? If it wasn't a hard requirement, I personally think the performance considerations here are more valuable. To be clear: I really like this feature; I just personally think the performance implications are more important 😕
Previously, devtools would (upon initialization) immediately register on-op event listeners on all provided root DDSs and immediately begin broadcasting data visualization messages on the window (regardless of whether or not the browser extension was running and listening).
This PR updates devtools to register these listeners lazily on demand. This leverages the existing laziness that was implemented for nested DDSs, such that we only register event listeners when we receive the
GET_DATA_VISAUALIZATIONmessage from the browser extension.A corresponding
CLOSE_DATA_VISUALIZATIONmessage has been added, which the extension now uses to indicate when it is done needing data from the client. The client leverages this alongside simple ref-counting to unsubscribe DDS listeners when there is no longer interest in data.Note that there are no cross-layer compat issues here between the devtools client library and the extension.
CLOSEmessage)CLOSEmessage)CLOSEhandler)CLOSE_DATA_VISUALIZATION. Still eager; view filters incomingDATA_VISUALIZATIONbyfluidObjectIdand detaches handlers on unmount, so extra/stale broadcasts are harmless. No regression.CLOSEhandler)GET_DATA_VISUALIZATION. Old view never sendsCLOSE, so each subscribed node stays subscribed after collapse — subscription leak, but behavior is no worse than the Old-core baseline (which monitored everything unconditionally).