Skip to content
3 changes: 2 additions & 1 deletion extensions/ipynb/extension.webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ module.exports = withDefaults({
context: __dirname,
entry: {
extension: './src/ipynbMain.ts',
notebookSerializerWorker: './src/notebookSerializerWorker.ts',
},
output: {
filename: 'ipynbMain.js'
filename: '[name].js'
}
});
6 changes: 6 additions & 0 deletions extensions/ipynb/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@
"scope": "resource",
"markdownDescription": "%ipynb.pasteImagesAsAttachments.enabled%",
"default": true
},
"ipynb.experimental.serialization": {
"type": "boolean",
"scope": "resource",
"markdownDescription": "%ipynb.experimental.serialization%",
"default": false
}
}
}
Expand Down
1 change: 1 addition & 0 deletions extensions/ipynb/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"displayName": ".ipynb Support",
"description": "Provides basic support for opening and reading Jupyter's .ipynb notebook files",
"ipynb.pasteImagesAsAttachments.enabled": "Enable/disable pasting of images into Markdown cells in ipynb notebook files. Pasted images are inserted as attachments to the cell.",
"ipynb.experimental.serialization": "Experimental feature to serialize the Jupyter notebook in a worker thread. Not supported when the Extension host is running as a web worker.",
"newUntitledIpynb.title": "New Jupyter Notebook",
"newUntitledIpynb.shortTitle": "Jupyter Notebook",
"openIpynbInNotebookEditor.title": "Open IPYNB File In Notebook Editor",
Expand Down
5 changes: 5 additions & 0 deletions extensions/ipynb/src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,8 @@ export interface CellMetadata {
*/
execution_count?: number;
}

export interface notebookSerializationWorkerData {
notebookContent: Partial<nbformat.INotebookContent>;
indentAmount: string;
}
4 changes: 2 additions & 2 deletions extensions/ipynb/src/ipynbMain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export function activate(context: vscode.ExtensionContext) {
get dropCustomMetadata() {
return true;
},
exportNotebook: (notebook: vscode.NotebookData): string => {
exportNotebook: (notebook: vscode.NotebookData): Promise<string> => {
return exportNotebook(notebook, serializer);
},
setNotebookMetadata: async (resource: vscode.Uri, metadata: Partial<NotebookMetadata>): Promise<boolean> => {
Expand All @@ -127,7 +127,7 @@ export function activate(context: vscode.ExtensionContext) {
};
}

function exportNotebook(notebook: vscode.NotebookData, serializer: NotebookSerializer): string {
function exportNotebook(notebook: vscode.NotebookData, serializer: NotebookSerializer): Promise<string> {
return serializer.serializeNotebookToString(notebook);
}

Expand Down
46 changes: 41 additions & 5 deletions extensions/ipynb/src/notebookSerializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { defaultNotebookFormat } from './constants';
import { getPreferredLanguage, jupyterNotebookModelToNotebookData } from './deserializers';
import { createJupyterCellFromNotebookCell, pruneCell, sortObjectPropertiesRecursively } from './serializers';
import * as fnv from '@enonic/fnv-plus';
import { notebookSerializationWorkerData } from './common';

export class NotebookSerializer implements vscode.NotebookSerializer {
constructor(readonly context: vscode.ExtensionContext) {
Expand Down Expand Up @@ -70,11 +71,46 @@ export class NotebookSerializer implements vscode.NotebookSerializer {
return data;
}

public serializeNotebook(data: vscode.NotebookData, _token: vscode.CancellationToken): Uint8Array {
return new TextEncoder().encode(this.serializeNotebookToString(data));
public async serializeNotebook(data: vscode.NotebookData, _token: vscode.CancellationToken): Promise<Uint8Array> {
return new TextEncoder().encode(await this.serializeNotebookToString(data));
}

public serializeNotebookToString(data: vscode.NotebookData): string {
private async serializeViaWorker(workerData: notebookSerializationWorkerData): Promise<string> {
const workerThreads = await import('node:worker_threads');
const path = await import('node:path');
const { Worker } = workerThreads;

return await new Promise((resolve, reject) => {
const workerFile = path.join(__dirname, 'notebookSerializerWorker.js');
const worker = new Worker(workerFile, { workerData });
worker.on('message', resolve);
worker.on('error', reject);
worker.on('exit', (code) => {
if (code !== 0) {
reject(new Error(`Worker stopped with exit code ${code}`));
}
});
});
}

private serializeNotebookToJSON(notebookContent: Partial<nbformat.INotebookContent>, indentAmount: string): Promise<string> {

const isInNodeJSContext = typeof process !== 'undefined' && process.release && process.release.name === 'node';
const experimentalSave = vscode.workspace.getConfiguration('ipynb').get('experimental.serialization', false);
if (isInNodeJSContext && experimentalSave) {
return this.serializeViaWorker({
notebookContent,
indentAmount
});
} else {
// ipynb always ends with a trailing new line (we add this so that SCMs do not show unnecessary changes, resulting from a missing trailing new line).
const sorted = sortObjectPropertiesRecursively(notebookContent);

return Promise.resolve(JSON.stringify(sorted, undefined, indentAmount) + '\n');
}
Comment on lines +96 to +110
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor Suggestion: Simplify Serialization Logic

The method serializeNotebookToJSON effectively handles conditional serialization logic based on the environment and configuration. However, consider simplifying the conditional branches to enhance readability and maintainability.

Here's a suggested refactor:

-private serializeNotebookToJSON(notebookContent: Partial<nbformat.INotebookContent>, indentAmount: string): Promise<string> {
-    const isInNodeJSContext = typeof process !== 'undefined' && process.release && process.release.name === 'node';
-    const experimentalSave = vscode.workspace.getConfiguration('ipynb').get('experimental.serialization', false);
-    if (isInNodeJSContext && experimentalSave) {
-        return this.serializeViaWorker({
-            notebookContent,
-            indentAmount
-        });
-    } else {
-        const sorted = sortObjectPropertiesRecursively(notebookContent);
-        return Promise.resolve(JSON.stringify(sorted, undefined, indentAmount) + '\n');
-    }
-}
+private async serializeNotebookToJSON(notebookContent: Partial<nbformat.INotebookContent>, indentAmount: string): Promise<string> {
+    if (this.shouldSerializeViaWorker()) {
+        return this.serializeViaWorker({ notebookContent, indentAmount });
+    }
+    const sorted = sortObjectPropertiesRecursively(notebookContent);
+    return JSON.stringify(sorted, undefined, indentAmount) + '\n';
+}
+
+private shouldSerializeViaWorker(): boolean {
+    return typeof process !== 'undefined' && process.release && process.release.name === 'node' &&
+           vscode.workspace.getConfiguration('ipynb').get('experimental.serialization', false);
+}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private serializeNotebookToJSON(notebookContent: Partial<nbformat.INotebookContent>, indentAmount: string): Promise<string> {
const isInNodeJSContext = typeof process !== 'undefined' && process.release && process.release.name === 'node';
const experimentalSave = vscode.workspace.getConfiguration('ipynb').get('experimental.serialization', false);
if (isInNodeJSContext && experimentalSave) {
return this.serializeViaWorker({
notebookContent,
indentAmount
});
} else {
// ipynb always ends with a trailing new line (we add this so that SCMs do not show unnecessary changes, resulting from a missing trailing new line).
const sorted = sortObjectPropertiesRecursively(notebookContent);
return Promise.resolve(JSON.stringify(sorted, undefined, indentAmount) + '\n');
}
private async serializeNotebookToJSON(notebookContent: Partial<nbformat.INotebookContent>, indentAmount: string): Promise<string> {
if (this.shouldSerializeViaWorker()) {
return this.serializeViaWorker({ notebookContent, indentAmount });
}
const sorted = sortObjectPropertiesRecursively(notebookContent);
return JSON.stringify(sorted, undefined, indentAmount) + '\n';
}
private shouldSerializeViaWorker(): boolean {
return typeof process !== 'undefined' && process.release && process.release.name === 'node' &&
vscode.workspace.getConfiguration('ipynb').get('experimental.serialization', false);
}

}

public serializeNotebookToString(data: vscode.NotebookData): Promise<string> {
const notebookContent = getNotebookMetadata(data);
// use the preferred language from document metadata or the first cell language as the notebook preferred cell language
const preferredCellLanguage = notebookContent.metadata?.language_info?.name ?? data.cells.find(cell => cell.kind === vscode.NotebookCellKind.Code)?.languageId;
Expand All @@ -86,8 +122,8 @@ export class NotebookSerializer implements vscode.NotebookSerializer {
const indentAmount = data.metadata && 'indentAmount' in data.metadata && typeof data.metadata.indentAmount === 'string' ?
data.metadata.indentAmount :
' ';
// ipynb always ends with a trailing new line (we add this so that SCMs do not show unnecessary changes, resulting from a missing trailing new line).
return JSON.stringify(sortObjectPropertiesRecursively(notebookContent), undefined, indentAmount) + '\n';

return this.serializeNotebookToJSON(notebookContent, indentAmount);
}
}

Expand Down
30 changes: 30 additions & 0 deletions extensions/ipynb/src/notebookSerializerWorker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { notebookSerializationWorkerData } from './common';
import { workerData, parentPort } from 'node:worker_threads';

function sortObjectPropertiesRecursively(obj: any): any {
if (Array.isArray(obj)) {
return obj.map(sortObjectPropertiesRecursively);
}
if (obj !== undefined && obj !== null && typeof obj === 'object' && Object.keys(obj).length > 0) {
return (
Object.keys(obj)
.sort()
.reduce<Record<string, any>>((sortedObj, prop) => {
sortedObj[prop] = sortObjectPropertiesRecursively(obj[prop]);
return sortedObj;
}, {}) as any
);
}
return obj;
}

if (parentPort) {
const { notebookContent, indentAmount } = <notebookSerializationWorkerData>workerData;
const json = JSON.stringify(sortObjectPropertiesRecursively(notebookContent), undefined, indentAmount) + '\n';
parentPort.postMessage(json);
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,17 @@ export class CellEditorStatusBar extends CellContentPart {
event: e
});
} else {
if ((e.target as HTMLElement).classList.contains('cell-status-item-has-command')) {
const target = e.target;
let itemHasCommand = false;
if (target && DOM.isHTMLElement(target)) {
const targetElement = <HTMLElement>target;
if (targetElement.classList.contains('cell-status-item-has-command')) {
itemHasCommand = true;
} else if (targetElement.parentElement && targetElement.parentElement.classList.contains('cell-status-item-has-command')) {
itemHasCommand = true;
}
}
if (itemHasCommand) {
Comment on lines +108 to +118
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved: Enhanced Command Detection Logic

The updated logic for detecting command-associated elements in the status bar is more robust, checking both the target and its parent. This change improves the accuracy and reliability of command detection, which is crucial for user interactions in the notebook environment.

Consider adding unit tests to ensure this new logic works as expected across different scenarios.

Would you like me to help with writing the unit tests for this new logic?

this._onDidClick.fire({
type: ClickTargetType.ContributedCommandItem,
event: e
Expand Down