Update Jupyter to new notebook preload message api #5775
Conversation
| ) | ||
| } | ||
| ] | ||
| new NotebookKernelPreload(Uri.file( |
There was a problem hiding this comment.
Minor change from new proposed. Needed to use the new class here.
| @@ -30,4 +29,4 @@ function initialize(api: NotebookRendererApi<any>) { | |||
| api.onWillDestroyOutput(disposeOutput); | |||
| } | |||
|
|
|||
| initialize(acquireNotebookRendererApi(JupyterIPyWidgetNotebookRenderer)); | |||
| initialize(acquireNotebookRendererApi()); | |||
There was a problem hiding this comment.
Another change from proposed, correct name is automatically passed.
| @@ -74,38 +84,70 @@ export class PostOffice implements IDisposable { | |||
| } | |||
|
|
|||
| public acquireApi(): IVsCodeApi | undefined { | |||
There was a problem hiding this comment.
Seems like this could be split up more? Or generalized into two different functions instead of multiple if points? I know some of it might be duped in each function, but I believe there's two distinct paths here?
- postKerneMessage = native case
- acquireVsCodeApi = webview case
There was a problem hiding this comment.
I'm suggesting:
- acquireApi (highest level)
- acquireNativeApi (for native case when new function - isNative - returns true). This function would do the work for bnding for the native case.
- acquireWebViewApi - This function would do the code we used to have?
There was a problem hiding this comment.
Were you thinking like just break this function up into the kernel / vs code specific functions? Or more like factor our a full common interface. Full interface feels kinda heavy, but it might read better.
There was a problem hiding this comment.
I think two different functions would be good enough and make it clearer which path you were on.
There was a problem hiding this comment.
I was actually starting down the path of an interface / class refactor. What do you think about this? @rchiodo
Codecov Report
@@ Coverage Diff @@
## main #5775 +/- ##
=======================================
+ Coverage 65% 72% +6%
=======================================
Files 352 403 +51
Lines 24148 26916 +2768
Branches 3637 3926 +289
=======================================
+ Hits 15818 19454 +3636
+ Misses 6939 5820 -1119
- Partials 1391 1642 +251
|
Thanks, updated to latest 7 version and regenerated. |
For #5753
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).