Merge pull request #3389 from jasongrout/forwardport

ibdafna · web-flow · commit 940c10da0e72 · 2022-02-18T09:40:37.000Z
Port #3337 and #3377 to master
diff --git 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
@@ -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
@@ -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<WidgetModel> {
+    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<WidgetModel> | 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<void> {
+  protected async _loadFromKernelModels(): Promise<void> {
     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<Private.ICommUpdateData>();
+        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<Private.ICommUpdateData>();
-          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<IManagerStateMap>((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
@@ -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
@@ -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<WidgetModel> | undefined;
+  get_model(model_id: string): Promise<WidgetModel>;
+
+  /**
+   * 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
@@ -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
@@ -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<widgets.WidgetModel> {
+    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<widgets.WidgetModel> | 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
@@ -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
diff --git a/python/jupyterlab_widgets/src/manager.ts b/python/jupyterlab_widgets/src/manager.ts
diff --git a/python/widgetsnbextension/src/extension.js b/python/widgetsnbextension/src/extension.js

Original file line number	Diff line number	Diff line change
`@@ -55,8 +55,8 @@ document.addEventListener('DOMContentLoaded', async function (event) {`
`55`	`55`	`const widgetData: any =`
`56`	`56`	`msg.content.data['application/vnd.jupyter.widget-view+json'];`
`57`	`57`	`if (widgetData !== undefined && widgetData.version_major === 2) {`
`58`		`- const model = await manager.get_model(widgetData.model_id);`
`59`		`- if (model !== undefined) {`
	`58`	`+ if (manager.has_model(widgetData.model_id)) {`
	`59`	`+ const model = await manager.get_model(widgetData.model_id)!;`
`60`	`60`	`manager.display_view(manager.create_view(model), widgetarea);`
`61`	`61`	`}`
`62`	`62`	`}`