diff --git a/docs/source/changelog.md b/docs/source/changelog.md index 860ecc786c..6e17ef357c 100644 --- a/docs/source/changelog.md +++ b/docs/source/changelog.md @@ -84,7 +84,7 @@ See also the - Fetch the full widget state via a control Comm ([#3021](https://github.com/jupyter-widgets/ipywidgets/pull/3021)) - Export LabWidgetManager and KernelWidgetManager ([#3166](https://github.com/jupyter-widgets/ipywidgets/pull/3166)) - More helpful semver range message ([#3185](https://github.com/jupyter-widgets/ipywidgets/pull/3185)) - +- Make the base widget manager `.get_model()` method always return a Promise, which is rejected if the requested model is not registered. To test if a model is registered, use the new `.has_model()` method ([#3389](https://github.com/jupyter-widgets/ipywidgets/pull/3389)) ### Documentation improvements - Documentation overhaul ([#3104](https://github.com/jupyter-widgets/ipywidgets/pull/3104), diff --git a/examples/web3/src/index.ts b/examples/web3/src/index.ts index f571e46489..36b5ed3338 100644 --- a/examples/web3/src/index.ts +++ b/examples/web3/src/index.ts @@ -55,8 +55,8 @@ document.addEventListener('DOMContentLoaded', async function (event) { const widgetData: any = msg.content.data['application/vnd.jupyter.widget-view+json']; if (widgetData !== undefined && widgetData.version_major === 2) { - const model = await manager.get_model(widgetData.model_id); - if (model !== undefined) { + if (manager.has_model(widgetData.model_id)) { + const model = await manager.get_model(widgetData.model_id)!; manager.display_view(manager.create_view(model), widgetarea); } } diff --git a/packages/base-manager/src/manager-base.ts b/packages/base-manager/src/manager-base.ts index 23d305f10b..78695083a6 100644 --- a/packages/base-manager/src/manager-base.ts +++ b/packages/base-manager/src/manager-base.ts @@ -210,15 +210,26 @@ export abstract class ManagerBase implements IWidgetManager { * Get a promise for a model by model id. * * #### Notes - * If a model is not found, undefined is returned (NOT a promise). However, - * the calling code should also deal with the case where a rejected promise - * is returned, and should treat that also as a model not found. + * If the model is not found, the returned Promise object is rejected. + * + * If you would like to synchronously test if a model exists, use .has_model(). + */ + async get_model(model_id: string): Promise { + const modelPromise = this._models[model_id]; + if (modelPromise === undefined) { + throw new Error('widget model not found'); + } + return modelPromise; + } + + /** + * Returns true if the given model is registered, otherwise false. + * + * #### Notes + * This is a synchronous way to check if a model is registered. */ - get_model(model_id: string): Promise | undefined { - // TODO: Perhaps we should return a Promise.reject if the model is not - // found. Right now this isn't a true async function because it doesn't - // always return a promise. - return this._models[model_id]; + has_model(model_id: string): boolean { + return this._models[model_id] !== undefined; } /** @@ -364,7 +375,7 @@ export abstract class ManagerBase implements IWidgetManager { /** * Fetch all widgets states from the kernel using the control comm channel * If this fails (control comm handler not implemented kernel side), - * it will fallback to `_loadFromKernelSlow`. + * it will fall back to `_loadFromKernelModels`. * * This is a utility function that can be used in subclasses. */ @@ -417,51 +428,67 @@ export abstract class ManagerBase implements IWidgetManager { initComm.close(); } catch (error) { console.warn( - 'Failed to fetch widgets through the "jupyter.widget.control" comm channel, fallback to slow fetching of widgets. Reason:', + 'Failed to fetch ipywidgets through the "jupyter.widget.control" comm channel, fallback to fetching individual model state. Reason:', error ); - // Fallback to the old implementation for old ipywidgets backend versions (<=7.6) - return this._loadFromKernelSlow(); + // Fall back to the old implementation for old ipywidgets backend versions (ipywidgets<=7.6) + return this._loadFromKernelModels(); } const states: any = data.states; - - // Extract buffer paths const bufferPaths: any = {}; - for (const bufferPath of data.buffer_paths) { - if (!bufferPaths[bufferPath[0]]) { - bufferPaths[bufferPath[0]] = []; + const bufferGroups: any = {}; + + // Group buffers and buffer paths by widget id + for (let i = 0; i < data.buffer_paths.length; i++) { + const [widget_id, ...path] = data.buffer_paths[i]; + const b = buffers[i]; + if (!bufferPaths[widget_id]) { + bufferPaths[widget_id] = []; + bufferGroups[widget_id] = []; } - bufferPaths[bufferPath[0]].push(bufferPath.slice(1)); + bufferPaths[widget_id].push(path); + bufferGroups[widget_id].push(b); } - // Start creating all widgets - await Promise.all( + // Create comms for all new widgets. + const widget_comms = await Promise.all( Object.keys(states).map(async (widget_id) => { + const comm = this.has_model(widget_id) + ? undefined + : await this._create_comm('jupyter.widget', widget_id); + return { widget_id, comm }; + }) + ); + + await Promise.all( + widget_comms.map(async ({ widget_id, comm }) => { + const state = states[widget_id]; + // Put binary buffers + if (widget_id in bufferPaths) { + put_buffers(state, bufferPaths[widget_id], bufferGroups[widget_id]); + } try { - const state = states[widget_id]; - const comm = await this._create_comm('jupyter.widget', widget_id); - - // Put binary buffers - if (widget_id in bufferPaths) { - const nBuffers = bufferPaths[widget_id].length; - put_buffers( - state, - bufferPaths[widget_id], - buffers.splice(0, nBuffers) + if (comm) { + // This must be the first await in the code path that + // reaches here so that registering the model promise in + // new_model can register the widget promise before it may + // be required by other widgets. + await this.new_model( + { + model_name: state.model_name, + model_module: state.model_module, + model_module_version: state.model_module_version, + model_id: widget_id, + comm: comm, + }, + state.state ); + } else { + // model already exists here + const model = await this.get_model(widget_id); + model!.set_state(state.state); } - - await this.new_model( - { - model_name: state.model_name, - model_module: state.model_module, - model_module_version: state.model_module_version, - model_id: widget_id, - comm: comm, - }, - state.state - ); } catch (error) { // Failed to create a widget model, we continue creating other models so that // other widgets can render @@ -472,63 +499,46 @@ export abstract class ManagerBase implements IWidgetManager { } /** - * Old implementation of fetching widgets one by one using + * Old implementation of fetching widget models one by one using * the request_state message on each comm. * * This is a utility function that can be used in subclasses. */ - protected async _loadFromKernelSlow(): Promise { + protected async _loadFromKernelModels(): Promise { const comm_ids = await this._get_comm_info(); // For each comm id that we do not know about, create the comm, and request the state. const widgets_info = await Promise.all( Object.keys(comm_ids).map(async (comm_id) => { - try { - const model = this.get_model(comm_id); - // TODO Have the same this.get_model implementation for - // the widgetsnbextension and labextension, the one that - // throws an error if the model is not found instead of - // returning undefined - if (model === undefined) { - throw new Error('widget model not found'); - } - await model; - // If we successfully get the model, do no more. + if (this.has_model(comm_id)) { return; - } catch (e) { - // If we have the widget model not found error, then we can create the - // widget. Otherwise, rethrow the error. We have to check the error - // message text explicitly because the get_model function in this - // class throws a generic error with this specific text. - if (e.message !== 'widget model not found') { - throw e; + } + + const comm = await this._create_comm(this.comm_target_name, comm_id); + + let msg_id = ''; + const info = new PromiseDelegate(); + comm.on_msg((msg: services.KernelMessage.ICommMsgMsg) => { + if ( + (msg.parent_header as any).msg_id === msg_id && + msg.header.msg_type === 'comm_msg' && + msg.content.data.method === 'update' + ) { + const data = msg.content.data as any; + const buffer_paths = data.buffer_paths || []; + const buffers = msg.buffers || []; + put_buffers(data.state, buffer_paths, buffers); + info.resolve({ comm, msg }); } - const comm = await this._create_comm(this.comm_target_name, comm_id); - - let msg_id = ''; - const info = new PromiseDelegate(); - comm.on_msg((msg: services.KernelMessage.ICommMsgMsg) => { - if ( - (msg.parent_header as any).msg_id === msg_id && - msg.header.msg_type === 'comm_msg' && - msg.content.data.method === 'update' - ) { - const data = msg.content.data as any; - const buffer_paths = data.buffer_paths || []; - const buffers = msg.buffers || []; - put_buffers(data.state, buffer_paths, buffers); - info.resolve({ comm, msg }); - } - }); - msg_id = comm.send( - { - method: 'request_state', - }, - this.callbacks(undefined) - ); + }); + msg_id = comm.send( + { + method: 'request_state', + }, + this.callbacks(undefined) + ); - return info.promise; - } + return info.promise; }) ); @@ -689,8 +699,8 @@ export abstract class ManagerBase implements IWidgetManager { // If the model has already been created, set its state and then // return it. - if (this._models[model_id] !== undefined) { - return this._models[model_id].then((model) => { + if (this.has_model(model_id)) { + return this.get_model(model_id)!.then((model) => { // deserialize state return (model.constructor as typeof WidgetModel) ._deserialize_state(modelState || {}, this) @@ -841,9 +851,7 @@ export abstract class ManagerBase implements IWidgetManager { protected filterExistingModelState(serialized_state: any): any { let models = serialized_state.state; models = Object.keys(models) - .filter((model_id) => { - return !this._models[model_id]; - }) + .filter((model_id) => !this.has_model(model_id)) .reduce((res, model_id) => { res[model_id] = models[model_id]; return res; diff --git a/packages/base-manager/test/src/manager_test.ts b/packages/base-manager/test/src/manager_test.ts index d913b982ca..47032e9d3a 100644 --- a/packages/base-manager/test/src/manager_test.ts +++ b/packages/base-manager/test/src/manager_test.ts @@ -147,8 +147,20 @@ describe('ManagerBase', function () { expect(await manager.get_model(model.model_id)).to.be.equal(model); }); - it('returns undefined when model is not registered', function () { - expect(this.managerBase.get_model('not-defined')).to.be.undefined; + it('returns rejected promise when model is not registered', function () { + expect(this.managerBase.get_model('not-defined')).to.be.rejected; + }); + }); + + describe('has_model', function () { + it('returns true when the model is registered', async function () { + const manager = this.managerBase; + const model = await manager.new_model(this.modelOptions); + expect(manager.has_model(model.model_id)).to.be.true; + }); + + it('returns false when the model is not registered', function () { + expect(this.managerBase.has_model('not-defined')).to.be.false; }); }); @@ -422,7 +434,7 @@ describe('ManagerBase', function () { const manager = this.managerBase; const model = await manager.new_model(spec); comm.close(); - expect(manager.get_model(model.model_id)).to.be.undefined; + expect(manager.get_model(model.model_id)).to.be.rejected; }); }); @@ -445,8 +457,8 @@ describe('ManagerBase', function () { expect(await manager.get_model(model1.model_id)).to.be.equal(model1); expect(await manager.get_model(model2.model_id)).to.be.equal(model2); await manager.clear_state(); - expect(manager.get_model(model1.model_id)).to.be.undefined; - expect(manager.get_model(model2.model_id)).to.be.undefined; + expect(manager.get_model(model1.model_id)).to.be.rejected; + expect(manager.get_model(model2.model_id)).to.be.rejected; expect((comm1.close as any).calledOnce).to.be.true; expect((comm2.close as any).calledOnce).to.be.true; expect(model1.comm).to.be.undefined; diff --git a/packages/base/src/manager.ts b/packages/base/src/manager.ts index ddefcae543..2c9c1317c6 100644 --- a/packages/base/src/manager.ts +++ b/packages/base/src/manager.ts @@ -123,7 +123,15 @@ export interface IWidgetManager { * the calling code should also deal with the case where a rejected promise * is returned, and should treat that also as a model not found. */ - get_model(model_id: string): Promise | undefined; + get_model(model_id: string): Promise; + + /** + * Returns true if the given model is registered, otherwise false. + * + * #### Notes + * This is a synchronous way to check if a model is registered. + */ + has_model(model_id: string): boolean; /** * Register a model instance promise with the manager. diff --git a/packages/base/src/widget.ts b/packages/base/src/widget.ts index f57110a312..dfa740e7a4 100644 --- a/packages/base/src/widget.ts +++ b/packages/base/src/widget.ts @@ -49,8 +49,8 @@ export function unpack_models( }); return utils.resolvePromisesDict(unpacked); } else if (typeof value === 'string' && value.slice(0, 10) === 'IPY_MODEL_') { - // get_model returns a promise already (except when it returns undefined!) - return Promise.resolve(manager.get_model(value.slice(10, value.length))); + // get_model returns a promise already + return manager.get_model(value.slice(10, value.length)); } else { return Promise.resolve(value); } diff --git a/packages/base/test/src/dummy-manager.ts b/packages/base/test/src/dummy-manager.ts index 792104b930..aafbc2624e 100644 --- a/packages/base/test/src/dummy-manager.ts +++ b/packages/base/test/src/dummy-manager.ts @@ -195,15 +195,26 @@ export class DummyManager implements widgets.IWidgetManager { * Get a promise for a model by model id. * * #### Notes - * If a model is not found, undefined is returned (NOT a promise). However, - * the calling code should also deal with the case where a rejected promise - * is returned, and should treat that also as a model not found. + * If the model is not found, the returned Promise object is rejected. + * + * If you would like to synchronously test if a model exists, use .has_model(). + */ + async get_model(model_id: string): Promise { + const modelPromise = this._models[model_id]; + if (modelPromise === undefined) { + throw new Error('widget model not found'); + } + return modelPromise; + } + + /** + * Returns true if the given model is registered, otherwise false. + * + * #### Notes + * This is a synchronous way to check if a model is registered. */ - get_model(model_id: string): Promise | undefined { - // TODO: Perhaps we should return a Promise.reject if the model is not - // found. Right now this isn't a true async function because it doesn't - // always return a promise. - return this._models[model_id]; + has_model(model_id: string): boolean { + return this._models[model_id] !== undefined; } /** diff --git a/packages/html-manager/src/libembed.ts b/packages/html-manager/src/libembed.ts index 317674cb63..494734d6aa 100644 --- a/packages/html-manager/src/libembed.ts +++ b/packages/html-manager/src/libembed.ts @@ -2,9 +2,10 @@ // Distributed under the terms of the Modified BSD License. declare let __webpack_public_path__: string; -// eslint-disable-next-line prefer-const +/* eslint-disable prefer-const, @typescript-eslint/no-unused-vars */ __webpack_public_path__ = (window as any).__jupyter_widgets_assets_path__ || __webpack_public_path__; +/* eslint-enable prefer-const, @typescript-eslint/no-unused-vars */ import '@fortawesome/fontawesome-free/css/all.min.css'; import '@fortawesome/fontawesome-free/css/v4-shims.min.css'; diff --git a/packages/html-manager/src/output_renderers.ts b/packages/html-manager/src/output_renderers.ts index 7aebfc9c9a..c343aa1201 100644 --- a/packages/html-manager/src/output_renderers.ts +++ b/packages/html-manager/src/output_renderers.ts @@ -19,21 +19,20 @@ export class WidgetRenderer extends Widget implements IRenderMime.IRenderer { async renderModel(model: IRenderMime.IMimeModel): Promise { const source: any = model.data[this.mimeType]; - const modelPromise = this._manager.get_model(source.model_id); - if (modelPromise) { - try { - const wModel = await modelPromise; - const wView = await this._manager.create_view(wModel); - Widget.attach(wView.luminoWidget, this.node); - } catch (err) { - console.log('Error displaying widget'); - console.log(err); - this.node.textContent = 'Error displaying widget'; - this.addClass('jupyter-widgets'); - } - } else { + if (!this._manager.has_model(source.model_id)) { this.node.textContent = 'Error creating widget: could not find model'; this.addClass('jupyter-widgets'); + return; + } + try { + const wModel = await this._manager.get_model(source.model_id); + const wView = await this._manager.create_view(wModel); + Widget.attach(wView.luminoWidget, this.node); + } catch (err) { + console.log('Error displaying widget'); + console.log(err); + this.node.textContent = 'Error displaying widget'; + this.addClass('jupyter-widgets'); } } diff --git a/python/jupyterlab_widgets/src/manager.ts b/python/jupyterlab_widgets/src/manager.ts index 8bd472d4f5..5432fe951e 100644 --- a/python/jupyterlab_widgets/src/manager.ts +++ b/python/jupyterlab_widgets/src/manager.ts @@ -262,21 +262,6 @@ export abstract class LabWidgetManager this._registry.set(data.name, data.version, data.exports); } - /** - * Get a model - * - * #### Notes - * Unlike super.get_model(), this implementation always returns a promise and - * never returns undefined. The promise will reject if the model is not found. - */ - async get_model(model_id: string): Promise { - const modelPromise = super.get_model(model_id); - if (modelPromise === undefined) { - throw new Error('widget model not found'); - } - return modelPromise; - } - /** * Register a widget model. */ diff --git a/python/widgetsnbextension/src/extension.js b/python/widgetsnbextension/src/extension.js index 7758efed9b..5d4228dfe2 100644 --- a/python/widgetsnbextension/src/extension.js +++ b/python/widgetsnbextension/src/extension.js @@ -150,9 +150,9 @@ function register_events(Jupyter, events, outputarea) { return; } - var model = manager.get_model(data.model_id); - if (model) { - model + if (manager.has_model(data.model_id)) { + manager + .get_model(data.model_id) .then(function (model) { return manager.create_view(model, { output: output }); })