Register notebook with VSC only in Insiders (#12681)

DonJayamanne · web-flow · commit 0ac275750dce · 2020-06-30T14:50:49.000-07:00
For #10496

If the API changes, and user is not using the VSC Notebooks or insiders, then swallow errors.
Do not register API against stable version of VSCode.
Also fixed a test.

Basically we need to ensure VSC works when using this API and not crash if the API changes.
diff --git a/src/client/common/application/notebook.ts b/src/client/common/application/notebook.ts
@@ -17,6 +17,7 @@ import { UseProposedApi } from '../constants';
 import { NotebookEditorSupport } from '../experiments/groups';
 import { IDisposableRegistry, IExperimentsManager } from '../types';
 import {
+    IApplicationEnvironment,
     IVSCodeNotebook,
     NotebookCellLanguageChangeEvent,
     NotebookCellOutputsChangeEvent,
@@ -26,24 +27,34 @@ import {
 @injectable()
 export class VSCodeNotebook implements IVSCodeNotebook {
     public get onDidChangeActiveNotebookEditor(): Event<NotebookEditor | undefined> {
-        return this.notebook.onDidChangeActiveNotebookEditor;
+        return this.canUseNotebookApi
+            ? this.notebook.onDidChangeActiveNotebookEditor
+            : new EventEmitter<NotebookEditor | undefined>().event;
     }
     public get onDidOpenNotebookDocument(): Event<NotebookDocument> {
-        return this.notebook.onDidOpenNotebookDocument;
+        return this.canUseNotebookApi
+            ? this.notebook.onDidOpenNotebookDocument
+            : new EventEmitter<NotebookDocument>().event;
     }
     public get onDidCloseNotebookDocument(): Event<NotebookDocument> {
-        return this.notebook.onDidCloseNotebookDocument;
+        return this.canUseNotebookApi
+            ? this.notebook.onDidCloseNotebookDocument
+            : new EventEmitter<NotebookDocument>().event;
     }
     public get notebookDocuments(): ReadonlyArray<NotebookDocument> {
-        return this.notebook.notebookDocuments;
+        return this.canUseNotebookApi ? this.notebook.notebookDocuments : [];
     }
     public get notebookEditors() {
-        return this.notebook.visibleNotebookEditors;
+        return this.canUseNotebookApi ? this.notebook.visibleNotebookEditors : [];
     }
     public get onDidChangeNotebookDocument(): Event<
         NotebookCellsChangeEvent | NotebookCellOutputsChangeEvent | NotebookCellLanguageChangeEvent
     > {
-        return this._onDidChangeNotebookDocument.event;
+        return this.canUseNotebookApi
+            ? this._onDidChangeNotebookDocument.event
+            : new EventEmitter<
+                  NotebookCellsChangeEvent | NotebookCellOutputsChangeEvent | NotebookCellLanguageChangeEvent
+              >().event;
     }
     public get activeNotebookEditor(): NotebookEditor | undefined {
         if (!this.useProposedApi) {
@@ -63,14 +74,21 @@ export class VSCodeNotebook implements IVSCodeNotebook {
     >();
     private addedEventHandlers?: boolean;
     private _notebook?: typeof notebook;
+    private readonly canUseNotebookApi?: boolean;
     private readonly handledCellChanges = new WeakSet<VSCNotebookCellsChangeEvent>();
     constructor(
         @inject(UseProposedApi) private readonly useProposedApi: boolean,
         @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
-        @inject(IExperimentsManager) readonly experimentManager: IExperimentsManager
+        @inject(IExperimentsManager) readonly experimentManager: IExperimentsManager,
+        @inject(IApplicationEnvironment) readonly env: IApplicationEnvironment
     ) {
-        if (this.useProposedApi && experimentManager.inExperiment(NotebookEditorSupport.nativeNotebookExperiment)) {
+        if (
+            this.useProposedApi &&
+            experimentManager.inExperiment(NotebookEditorSupport.nativeNotebookExperiment) &&
+            this.env.channel === 'insiders'
+        ) {
             this.addEventHandlers();
+            this.canUseNotebookApi = true;
         }
     }
     public registerNotebookContentProvider(notebookType: string, provider: NotebookContentProvider): Disposable {
diff --git a/src/client/datascience/notebook/integration.ts b/src/client/datascience/notebook/integration.ts
@@ -11,6 +11,7 @@ import {
     IVSCodeNotebook
 } from '../../common/application/types';
 import { NotebookEditorSupport } from '../../common/experiments/groups';
+import { traceError } from '../../common/logger';
 import { IFileSystem } from '../../common/platform/types';
 import { IDisposableRegistry, IExperimentsManager, IExtensionContext } from '../../common/types';
 import { DataScience } from '../../common/utils/localize';
@@ -63,38 +64,49 @@ export class NotebookIntegration implements IExtensionSingleActivationService {
         if (this.experiment.inExperiment(NotebookEditorSupport.nativeNotebookExperiment)) {
             await this.enableNotebooks(true);
         }
-        this.disposables.push(
-            this.vscNotebook.registerNotebookContentProvider(JupyterNotebookView, this.notebookContentProvider)
-        );
-        this.disposables.push(
-            this.vscNotebook.registerNotebookKernel(JupyterNotebookView, ['**/*.ipynb'], this.notebookKernel)
-        );
-        this.disposables.push(
-            this.vscNotebook.registerNotebookOutputRenderer(
-                'jupyter-notebook-renderer',
-                {
-                    mimeTypes: [
-                        'application/geo+json',
-                        'application/vdom.v1+json',
-                        'application/vnd.dataresource+json',
-                        'application/vnd.plotly.v1+json',
-                        'application/vnd.vega.v2+json',
-                        'application/vnd.vega.v3+json',
-                        'application/vnd.vega.v4+json',
-                        'application/vnd.vega.v5+json',
-                        'application/vnd.vegalite.v1+json',
-                        'application/vnd.vegalite.v2+json',
-                        'application/vnd.vegalite.v3+json',
-                        'application/vnd.vegalite.v4+json',
-                        'application/x-nteract-model-debug+json',
-                        'image/gif',
-                        'text/latex',
-                        'text/vnd.plotly.v1+html'
-                    ]
-                },
-                this.renderer
-            )
-        );
+        if (this.env.channel !== 'insiders') {
+            return;
+        }
+        try {
+            this.disposables.push(
+                this.vscNotebook.registerNotebookContentProvider(JupyterNotebookView, this.notebookContentProvider)
+            );
+            this.disposables.push(
+                this.vscNotebook.registerNotebookKernel(JupyterNotebookView, ['**/*.ipynb'], this.notebookKernel)
+            );
+            this.disposables.push(
+                this.vscNotebook.registerNotebookOutputRenderer(
+                    'jupyter-notebook-renderer',
+                    {
+                        mimeTypes: [
+                            'application/geo+json',
+                            'application/vdom.v1+json',
+                            'application/vnd.dataresource+json',
+                            'application/vnd.plotly.v1+json',
+                            'application/vnd.vega.v2+json',
+                            'application/vnd.vega.v3+json',
+                            'application/vnd.vega.v4+json',
+                            'application/vnd.vega.v5+json',
+                            'application/vnd.vegalite.v1+json',
+                            'application/vnd.vegalite.v2+json',
+                            'application/vnd.vegalite.v3+json',
+                            'application/vnd.vegalite.v4+json',
+                            'application/x-nteract-model-debug+json',
+                            'image/gif',
+                            'text/latex',
+                            'text/vnd.plotly.v1+html'
+                        ]
+                    },
+                    this.renderer
+                )
+            );
+        } catch (ex) {
+            // If something goes wrong, and we're not in Insiders & not using the NativeEditor experiment, then swallow errors.
+            traceError('Failed to register VS Code Notebook API', ex);
+            if (this.experiment.inExperiment(NotebookEditorSupport.nativeNotebookExperiment)) {
+                throw ex;
+            }
+        }
     }
     private async enableNotebooks(useVSCodeNotebookAsDefaultEditor: boolean) {
         if (this.env.channel === 'stable') {
diff --git a/src/test/datascience/notebook/helper.ts b/src/test/datascience/notebook/helper.ts
@@ -9,7 +9,7 @@ import * as fs from 'fs-extra';
 import * as path from 'path';
 import * as sinon from 'sinon';
 import * as tmp from 'tmp';
-import { commands } from 'vscode';
+import { commands, Uri } from 'vscode';
 import { NotebookCell } from '../../../../types/vscode-proposed';
 import { CellDisplayOutput } from '../../../../typings/vscode-proposed';
 import { IApplicationEnvironment, IVSCodeNotebook } from '../../../client/common/application/types';
@@ -198,26 +198,40 @@ export async function startJupyter() {
     await commands.executeCommand('workbench.action.files.saveAll');
     await closeActiveWindows();
 
-    // Create a new nb, add a python cell and execute it.
-    // Doing that will start jupyter.
-    await editorProvider.createNew();
-    await (await insertPythonCell('print("Hello World")', 0)).waitForCellToGetAdded();
-    const model = editorProvider.activeEditor?.model;
-    editorProvider.activeEditor?.runAllCells();
-    // Wait for 15s for Jupyter to start.
-    await waitForCondition(async () => (model?.cells[0].data.outputs as []).length > 0, 15_000, 'Cell not executed');
-
-    const saveStub = sinon.stub(contentProvider, 'saveNotebook');
-    const saveAsStub = sinon.stub(contentProvider, 'saveNotebookAs');
+    const disposables: IDisposable[] = [];
     try {
+        const templateIPynb = path.join(
+            EXTENSION_ROOT_DIR_FOR_TESTS,
+            'src',
+            'test',
+            'datascience',
+            'notebook',
+            'empty.ipynb'
+        );
+        const tempIPynb = await createTemporaryNotebook(templateIPynb, disposables);
+        await editorProvider.open(Uri.file(tempIPynb));
+        await (await insertPythonCell('print("Hello World")', 0)).waitForCellToGetAdded();
+        const model = editorProvider.activeEditor?.model;
+        editorProvider.activeEditor?.runAllCells();
+        // Wait for 15s for Jupyter to start.
+        await waitForCondition(
+            async () => (model?.cells[0].data.outputs as []).length > 0,
+            15_000,
+            'Cell not executed'
+        );
+
+        const saveStub = sinon.stub(contentProvider, 'saveNotebook');
+        const saveAsStub = sinon.stub(contentProvider, 'saveNotebookAs');
         // We cannot close notebooks if there are any uncommitted changes (UI could hang with prompts etc).
         saveStub.callsFake(noop as any);
         saveAsStub.callsFake(noop as any);
+        disposables.push({
+            dispose: () => saveStub.restore()
+        });
         await commands.executeCommand('workbench.action.files.saveAll');
         await closeActiveWindows();
     } finally {
-        saveStub.restore();
-        saveAsStub.restore();
+        disposables.forEach((d) => d.dispose());
     }
 }
 
