You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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_VISAUALIZATION message from the browser extension.
A corresponding CLOSE_DATA_VISUALIZATION message 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: this PR does not address the potential listener leak that can occur if a consumer requests data events but fails to send the corresponding close message. But since the code previously never de-registered listeners once initially registered, this is still a strict improvement. We can follow up on this in the future as needed.
Note that there are no cross-layer compat issues here between the devtools client library and the extension.
client ↓ \ extension →
Old extension (doesn't send CLOSE message)
New extension (sends CLOSE message)
Old client (eager broadcast, no CLOSE handler)
Baseline. Eager broadcasting of all reachable roots; subscriptions never released. Works as before.
Compatible. Old core ignores the unrecognized CLOSE_DATA_VISUALIZATION. Still eager; view filters incoming DATA_VISUALIZATION by fluidObjectId and detaches handlers on unmount, so extra/stale broadcasts are harmless. No regression.
New client (on-demand, ref-counted, CLOSE handler)
Compatible, strict improvement. Nothing broadcasts until the view sends GET_DATA_VISUALIZATION. Old view never sends CLOSE, so each subscribed node stays subscribed after collapse — subscription leak, but behavior is no worse than the Old-core baseline (which monitored everything unconditionally).
Fully intended behavior. On-demand subscribe on expand/mount, balanced unsubscribe on collapse/unmount. Monitoring (and broadcasting) occurs only while a view is actively displaying an object. No leak (assuming proper view teardown).
NIT: While renaming existing types to DataVisualizerEvents, there's a dangling reference to {@link SharedObjectListenerEvents | "update"} event at Line 511 of DataVisualization.ts. Might as well fix it here for consistency
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).