Fix support for IPyWidgets in Interactive Window #4208
Conversation
DonJayamanne
requested review from
DavidKutu,
greazer,
IanMatthewHuff,
joyceerhl and
rchiodo
src/client/datascience/ipywidgets/notebookIPyWidgetCoordinator.ts
Outdated
Show resolved
Hide resolved
src/client/datascience/ipywidgets/webviewIPyWidgetCoordinator.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #4208 +/- ##
=======================================
+ Coverage 0 73% +73%
=======================================
Files 0 384 +384
Lines 0 25141 +25141
Branches 0 3595 +3595
=======================================
+ Hits 0 18504 +18504
- Misses 0 5220 +5220
- Partials 0 1417 +1417
|
| @@ -94,6 +92,19 @@ export class CommonMessageCoordinator { | |||
| this.getIPyWidgetScriptSource()?.onMessage(message, payload); | |||
| } | |||
|
|
|||
| public async initialize() { | |||
There was a problem hiding this comment.
Does this function need to be async? It does not seem to use await anywhere.
| const result = new CommonMessageCoordinator(identity, serviceContainer); | ||
| await result.initialize(); | ||
| return result; | ||
| public static create(identity: Uri, serviceContainer: IServiceContainer): CommonMessageCoordinator { |
There was a problem hiding this comment.
The purpose of this factory method was to ensure that initialize is called once (and only once) after creation and that the object is not used until the initialization is done. By taking out the call to initialize and making initialize public, we now rely on the user of the class to be diligent and to not:
- call
initializetwice - use the object before calling
initialize - use the object before the call to
initializefinishes
| @@ -62,7 +62,7 @@ export class WebviewIPyWidgetCoordinator implements IInteractiveWindowListener { | |||
| // There should be an instance of the WebviewMessageCoordinator per notebook webview or interactive window. Create | |||
| // the message coordinator as soon as we're sure what notebook we're in. | |||
| this.notebookIdentity = args.resource; | |||
| this.messageCoordinator = await CommonMessageCoordinator.create(this.notebookIdentity, this.serviceContainer); | |||
| this.messageCoordinator = CommonMessageCoordinator.create(this.notebookIdentity, this.serviceContainer); | |||
There was a problem hiding this comment.
It seems that the reason for this change was to allow for a specialization of the postEmitter.event of the CommonMessageCoordinator before the initialization takes place.
To avoid making initialize public and relying on end-user cooperation, I would recommend modifying the factory method create to take an optional parameter that allows the caller to inject the postEmitter.
|
For convenience, I opened #4273 addressing these comments. |
For #4203