From b10e675bbc76bd46f674e860b7a77d52f94162b4 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 18 Nov 2024 16:02:41 -0600 Subject: [PATCH] Register cleanup methods when widget gets initialized in a reactive context --- js/src/comm.ts | 3 +-- js/src/output.ts | 6 ------ shinywidgets/_comm.py | 8 +++++--- shinywidgets/_render_widget_base.py | 27 --------------------------- shinywidgets/_shinywidgets.py | 28 +++++++++++++--------------- shinywidgets/static/output.js | 4 ++-- 6 files changed, 21 insertions(+), 55 deletions(-) diff --git a/js/src/comm.ts b/js/src/comm.ts index 9805ebb..3c1c7b0 100644 --- a/js/src/comm.ts +++ b/js/src/comm.ts @@ -96,8 +96,7 @@ export class ShinyComm { metadata?: any, buffers?: ArrayBuffer[] | ArrayBufferView[] ): string { - // Inform the server that a client-side model/comm has closed - Shiny.setInputValue("shinywidgets_comm_close", this.comm_id, {priority: "event"}); + // I don't think we need to do anything here? return this.comm_id; } diff --git a/js/src/output.ts b/js/src/output.ts index 9a03721..32adc56 100644 --- a/js/src/output.ts +++ b/js/src/output.ts @@ -215,12 +215,6 @@ Shiny.addCustomMessageHandler("shinywidgets_comm_close", (msg_txt) => { }); }); -// At least currently, all widgets must be created within a session scope, so we can -// clear the state (i.e., .close() all the widget models) when the session ends -$(document).on("shiny:disconnected", () => { - manager.clear_state(); -}); - // Our version of https://github.com/jupyter-widgets/widget-cookiecutter/blob/9694718/%7B%7Bcookiecutter.github_project_name%7D%7D/js/lib/extension.js#L8 function setBaseURL(x: string = '') { const base_url = document.querySelector('body').getAttribute('data-base-url'); diff --git a/shinywidgets/_comm.py b/shinywidgets/_comm.py index b9166cc..d903255 100644 --- a/shinywidgets/_comm.py +++ b/shinywidgets/_comm.py @@ -97,9 +97,11 @@ def close( return self._closed = True data = self._closed_data if data is None else data - self._publish_msg( - "shinywidgets_comm_close", data=data, metadata=metadata, buffers=buffers - ) + # If there's no active session, we can't send a message to the client + if get_current_session(): + self._publish_msg( + "shinywidgets_comm_close", data=data, metadata=metadata, buffers=buffers + ) if not deleting: # If deleting, the comm can't be unregistered self.comm_manager.unregister_comm(self) diff --git a/shinywidgets/_render_widget_base.py b/shinywidgets/_render_widget_base.py index cf81ee1..0cb3083 100644 --- a/shinywidgets/_render_widget_base.py +++ b/shinywidgets/_render_widget_base.py @@ -70,9 +70,6 @@ def __init__( self._contexts: set[Context] = set() async def render(self) -> Jsonifiable | None: - # Before executing user function, get currently active widgets - widgets_before = set(WIDGET_INSTANCE_MAP.keys()) - value = await self.fn() # Attach value/widget attributes to user func so they can be accessed (in other reactive contexts) @@ -91,16 +88,6 @@ async def render(self) -> Jsonifiable | None: self._widget = cast(WidgetT, widget) - # Collect widget ids introduced by the render function - widgets_after = set(WIDGET_INSTANCE_MAP.keys()) - widgets_diff = widgets_after - widgets_before - - # The next time this render context gets invalidated, close widgets introduced - # by the user function. This _could_ be problematic if widgets get hoisted - # out of the render function for use elsewhere, but if you're doing that, - # you're likely to have other problems anyway. - get_current_context().on_invalidate(lambda: close_widgets(widgets_diff)) - # Don't actually display anything unless this is a DOMWidget if not isinstance(widget, DOMWidget): return None @@ -226,17 +213,3 @@ def set_layout_defaults(widget: Widget) -> Tuple[Widget, bool]: widget.chart = chart return (widget, fill) - - -# Dictionary of all "active" widgets (ipywidgets automatically adds to this dictionary as -# new widgets are created, but they won't get removed until the widget is explictly closed) -WIDGET_INSTANCE_MAP = cast(dict[str, Widget], Widget.widgets) # pyright: ignore[reportUnknownMemberType] - -# Given a set of widget ids, close those widgets. Closing should not only -# remove the widget reference, but also send a message to the client to remove -# the corresponding model from the widget manager. -def close_widgets(ids: set[str]): - for id in ids: - widget = WIDGET_INSTANCE_MAP.get(id) - if widget is not None: - widget.close() diff --git a/shinywidgets/_shinywidgets.py b/shinywidgets/_shinywidgets.py index 679ccf8..78c56cf 100644 --- a/shinywidgets/_shinywidgets.py +++ b/shinywidgets/_shinywidgets.py @@ -26,7 +26,7 @@ from ._cdn import SHINYWIDGETS_CDN_ONLY, SHINYWIDGETS_EXTENSION_WARNING from ._comm import BufferType, ShinyComm, ShinyCommManager from ._dependencies import require_dependency -from ._render_widget_base import WIDGET_INSTANCE_MAP +from ._render_widget_base import has_current_context from ._utils import is_instance_of_class, package_dir __all__ = ( @@ -94,6 +94,15 @@ def init_shiny_widget(w: Widget): html_deps=session._process_ui(TagList(widget_dep))["deps"], ) + # If we're in a reactive context, close this widget when the context is invalidated + if has_current_context(): + ctx = get_current_context() + ctx.on_invalidate(lambda: w.close()) + + # Also close the widget when the session ends (if the widget is already closed, this + # is a no-op) + session.on_ended(lambda: w.close()) + # Some widget's JS make external requests for static files (e.g., # ipyleaflet markers) under this resource path. Note that this assumes that # we're setting the data-base-url attribute on the (which we should @@ -111,7 +120,7 @@ def init_shiny_widget(w: Widget): # everything after this point should be done once per session if session in SESSIONS: return - SESSIONS.add(session) # type: ignore + SESSIONS.add(session) # Somewhere inside ipywidgets, it makes requests for static files # under the publicPath set by the webpack.config.js file. @@ -133,21 +142,10 @@ def _(): comm: ShinyComm = COMM_MANAGER.comms[comm_id] comm.handle_msg(msg) - # Handle a close message from the client. - @reactive.effect - @reactive.event(session.input.shinywidgets_comm_close) - def _(): - comm_id = session.input.shinywidgets_comm_close() - # Close the widget, which unregisters/deletes the comm, and also drops - # ipywidget's reference to the instance, allowing it to be garbage collected. - w_obj = WIDGET_INSTANCE_MAP.get(comm_id) - if w_obj: - w_obj.close() - def _restore_state(): if old_comm_klass is not None: Widget.comm.klass = old_comm_klass # type: ignore - SESSIONS.remove(session) # type: ignore + SESSIONS.remove(session) session.on_ended(_restore_state) @@ -156,7 +154,7 @@ def _restore_state(): Widget.on_widget_constructed(init_shiny_widget) # type: ignore # Use WeakSet() over Set() so that the session can be garbage collected -SESSIONS = WeakSet() # type: ignore +SESSIONS: WeakSet[Session] = WeakSet() COMM_MANAGER = ShinyCommManager() diff --git a/shinywidgets/static/output.js b/shinywidgets/static/output.js index 65bc82a..829cd0d 100644 --- a/shinywidgets/static/output.js +++ b/shinywidgets/static/output.js @@ -26,7 +26,7 @@ eval("__webpack_require__.r(__webpack_exports__);\n/* harmony export */ __webpac \*********************/ /***/ ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => { -eval("__webpack_require__.r(__webpack_exports__);\n/* harmony export */ __webpack_require__.d(__webpack_exports__, {\n/* harmony export */ \"ShinyComm\": () => (/* binding */ ShinyComm)\n/* harmony export */ });\n/* harmony import */ var _utils__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! ./utils */ \"./src/utils.ts\");\n\n// This class is a striped down version of Comm from @jupyter-widgets/base\n// https://github.com/jupyter-widgets/ipywidgets/blob/88cec8/packages/base/src/services-shim.ts#L192-L335\n// Note that the Kernel.IComm implementation is located here\n// https://github.com/jupyterlab/jupyterlab/blob/master/packages/services/src/kernel/comm.ts\nclass ShinyComm {\n constructor(model_id) {\n this.comm_id = model_id;\n // TODO: make this configurable (see comments in send() below)?\n this.throttler = new _utils__WEBPACK_IMPORTED_MODULE_0__.Throttler(100);\n }\n // This might not be needed\n get target_name() {\n return \"jupyter.widgets\";\n }\n send(data, callbacks, metadata, buffers) {\n const msg = {\n content: { comm_id: this.comm_id, data: data },\n metadata: metadata,\n // TODO: need to _encode_ any buffers into base64 (JSON.stringify just drops them)\n buffers: buffers || [],\n // this doesn't seem relevant to the widget?\n header: {}\n };\n const msg_txt = JSON.stringify(msg);\n // Since ipyleaflet can send mousemove events very quickly when hovering over the map,\n // we throttle them to ensure that the server doesn't get overwhelmed. Said events\n // generate a payload that looks like this:\n // {\"method\": \"custom\", \"content\": {\"event\": \"interaction\", \"type\": \"mousemove\", \"coordinates\": [-17.76259815404015, 12.096729340756617]}}\n //\n // TODO: This is definitely not ideal. It would be better to have a way to specify/\n // customize throttle rates instead of having such a targetted fix for ipyleaflet.\n const is_mousemove = data.method === \"custom\" &&\n data.content.event === \"interaction\" &&\n data.content.type === \"mousemove\";\n if (is_mousemove) {\n this.throttler.throttle(() => {\n Shiny.setInputValue(\"shinywidgets_comm_send\", msg_txt, { priority: \"event\" });\n });\n }\n else {\n this.throttler.flush();\n Shiny.setInputValue(\"shinywidgets_comm_send\", msg_txt, { priority: \"event\" });\n }\n // When client-side changes happen to the WidgetModel, this send method\n // won't get called for _every_ change (just the first one). The\n // expectation is that this method will eventually end up calling itself\n // (via callbacks) when the server is ready (i.e., idle) to receive more\n // updates. To make sense of this, see\n // https://github.com/jupyter-widgets/ipywidgets/blob/88cec8b/packages/base/src/widget.ts#L550-L557\n if (callbacks && callbacks.iopub && callbacks.iopub.status) {\n setTimeout(() => {\n // TODO-future: it doesn't seem quite right to report that shiny is always idle.\n // Maybe listen to the shiny-busy flag?\n // const state = document.querySelector(\"html\").classList.contains(\"shiny-busy\") ? \"busy\" : \"idle\";\n const msg = { content: { execution_state: \"idle\" } };\n callbacks.iopub.status(msg);\n }, 0);\n }\n return this.comm_id;\n }\n open(data, callbacks, metadata, buffers) {\n // I don't think we need to do anything here?\n return this.comm_id;\n }\n close(data, callbacks, metadata, buffers) {\n // Inform the server that a client-side model/comm has closed\n Shiny.setInputValue(\"shinywidgets_comm_close\", this.comm_id, { priority: \"event\" });\n return this.comm_id;\n }\n on_msg(callback) {\n this._msg_callback = callback.bind(this);\n }\n on_close(callback) {\n this._close_callback = callback.bind(this);\n }\n handle_msg(msg) {\n if (this._msg_callback)\n this._msg_callback(msg);\n }\n handle_close(msg) {\n if (this._close_callback)\n this._close_callback(msg);\n }\n}\n\n\n//# sourceURL=webpack://@jupyter-widgets/shiny-embed-manager/./src/comm.ts?"); +eval("__webpack_require__.r(__webpack_exports__);\n/* harmony export */ __webpack_require__.d(__webpack_exports__, {\n/* harmony export */ \"ShinyComm\": () => (/* binding */ ShinyComm)\n/* harmony export */ });\n/* harmony import */ var _utils__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! ./utils */ \"./src/utils.ts\");\n\n// This class is a striped down version of Comm from @jupyter-widgets/base\n// https://github.com/jupyter-widgets/ipywidgets/blob/88cec8/packages/base/src/services-shim.ts#L192-L335\n// Note that the Kernel.IComm implementation is located here\n// https://github.com/jupyterlab/jupyterlab/blob/master/packages/services/src/kernel/comm.ts\nclass ShinyComm {\n constructor(model_id) {\n this.comm_id = model_id;\n // TODO: make this configurable (see comments in send() below)?\n this.throttler = new _utils__WEBPACK_IMPORTED_MODULE_0__.Throttler(100);\n }\n // This might not be needed\n get target_name() {\n return \"jupyter.widgets\";\n }\n send(data, callbacks, metadata, buffers) {\n const msg = {\n content: { comm_id: this.comm_id, data: data },\n metadata: metadata,\n // TODO: need to _encode_ any buffers into base64 (JSON.stringify just drops them)\n buffers: buffers || [],\n // this doesn't seem relevant to the widget?\n header: {}\n };\n const msg_txt = JSON.stringify(msg);\n // Since ipyleaflet can send mousemove events very quickly when hovering over the map,\n // we throttle them to ensure that the server doesn't get overwhelmed. Said events\n // generate a payload that looks like this:\n // {\"method\": \"custom\", \"content\": {\"event\": \"interaction\", \"type\": \"mousemove\", \"coordinates\": [-17.76259815404015, 12.096729340756617]}}\n //\n // TODO: This is definitely not ideal. It would be better to have a way to specify/\n // customize throttle rates instead of having such a targetted fix for ipyleaflet.\n const is_mousemove = data.method === \"custom\" &&\n data.content.event === \"interaction\" &&\n data.content.type === \"mousemove\";\n if (is_mousemove) {\n this.throttler.throttle(() => {\n Shiny.setInputValue(\"shinywidgets_comm_send\", msg_txt, { priority: \"event\" });\n });\n }\n else {\n this.throttler.flush();\n Shiny.setInputValue(\"shinywidgets_comm_send\", msg_txt, { priority: \"event\" });\n }\n // When client-side changes happen to the WidgetModel, this send method\n // won't get called for _every_ change (just the first one). The\n // expectation is that this method will eventually end up calling itself\n // (via callbacks) when the server is ready (i.e., idle) to receive more\n // updates. To make sense of this, see\n // https://github.com/jupyter-widgets/ipywidgets/blob/88cec8b/packages/base/src/widget.ts#L550-L557\n if (callbacks && callbacks.iopub && callbacks.iopub.status) {\n setTimeout(() => {\n // TODO-future: it doesn't seem quite right to report that shiny is always idle.\n // Maybe listen to the shiny-busy flag?\n // const state = document.querySelector(\"html\").classList.contains(\"shiny-busy\") ? \"busy\" : \"idle\";\n const msg = { content: { execution_state: \"idle\" } };\n callbacks.iopub.status(msg);\n }, 0);\n }\n return this.comm_id;\n }\n open(data, callbacks, metadata, buffers) {\n // I don't think we need to do anything here?\n return this.comm_id;\n }\n close(data, callbacks, metadata, buffers) {\n // I don't think we need to do anything here?\n return this.comm_id;\n }\n on_msg(callback) {\n this._msg_callback = callback.bind(this);\n }\n on_close(callback) {\n this._close_callback = callback.bind(this);\n }\n handle_msg(msg) {\n if (this._msg_callback)\n this._msg_callback(msg);\n }\n handle_close(msg) {\n if (this._close_callback)\n this._close_callback(msg);\n }\n}\n\n\n//# sourceURL=webpack://@jupyter-widgets/shiny-embed-manager/./src/comm.ts?"); /***/ }), @@ -36,7 +36,7 @@ eval("__webpack_require__.r(__webpack_exports__);\n/* harmony export */ __webpac \***********************/ /***/ ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => { -eval("__webpack_require__.r(__webpack_exports__);\n/* harmony import */ var _jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! @jupyter-widgets/html-manager */ \"@jupyter-widgets/html-manager\");\n/* harmony import */ var _jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0___default = /*#__PURE__*/__webpack_require__.n(_jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0__);\n/* harmony import */ var _comm__WEBPACK_IMPORTED_MODULE_1__ = __webpack_require__(/*! ./comm */ \"./src/comm.ts\");\n/* harmony import */ var _utils__WEBPACK_IMPORTED_MODULE_2__ = __webpack_require__(/*! ./utils */ \"./src/utils.ts\");\nvar _a;\n\n\n\n/******************************************************************************\n * Define a custom HTMLManager for use with Shiny\n ******************************************************************************/\nclass OutputManager extends _jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0__.HTMLManager {\n // In a soon-to-be-released version of @jupyter-widgets/html-manager,\n // display_view()'s first \"dummy\" argument will be removed... this shim simply\n // makes it so that our manager can work with either version\n // https://github.com/jupyter-widgets/ipywidgets/commit/159bbe4#diff-45c126b24c3c43d2cee5313364805c025e911c4721d45ff8a68356a215bfb6c8R42-R43\n async display_view(view, options) {\n const n_args = super.display_view.length;\n if (n_args === 3) {\n return super.display_view({}, view, options);\n }\n else {\n // @ts-ignore\n return super.display_view(view, options);\n }\n }\n}\n// Define our own custom module loader for Shiny\nconst shinyRequireLoader = async function (moduleName, moduleVersion) {\n // shiny provides require.js and also sets `define.amd=false` to prevent