Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📢 Notebook API announcements #93265

Closed
rebornix opened this issue Mar 23, 2020 · 116 comments
Closed

📢 Notebook API announcements #93265

rebornix opened this issue Mar 23, 2020 · 116 comments
Assignees
Labels
api notebook notebook-api plan-item VS Code - planned item for upcoming
Milestone

Comments

@rebornix
Copy link
Member

We introduced a proposed API for Notebook but currently the API is majorly two managed object: NotebookDocument and NotebookCell. We create them for extensions and listen to their properties changes. However this doesn't follow the principal of TextEditor/Document where TextDocument is always readonly and TextEditor is the API for applying changes to the document.

If we try to follow TextEditor/Document, the API can be shaped as below

export interface NotebookEditor {
    readonly document: NotebookDocument;
    viewColumn?: ViewColumn;
    /**
     * Fired when the output hosting webview posts a message.
     */
    readonly onDidReceiveMessage: Event<any>;
    /**
     * Post a message to the output hosting webview.
     *
     * Messages are only delivered if the editor is live.
     *
     * @param message Body of the message. This must be a string or other json serilizable object.
     */
    postMessage(message: any): Thenable<boolean>;

    /**
     * Create a notebook cell. The cell is not inserted into current document when created. Extensions should insert the cell into the document by [TextDocument.cells](#TextDocument.cells)
     */
    createCell(content: string, language: string, type: CellKind, outputs: CellOutput[], metadata: NotebookCellMetadata): NotebookCell;

    /**
     * Insert/Delete cells from the document
     */
    spliceCells(index: number, deleteCnt: number, insertedCells: NotebookCell[]): Promise<void>;

    /**
     * Make changes to individual cells
     */
    applyCellEdits(cell: NotebookCell, changes: { language?: string, outputs?: CellOutput[], metadata?: NotebookCellMetadata }): Promise<void>;
}

export interface NotebookDocument {
    readonly uri: Uri;
    readonly fileName: string;
    readonly isDirty: boolean;
    readonly languages: string[];
    readonly cells: NotebookCell[];
    readonly displayOrder?: GlobPattern[];
    readonly metadata?: NotebookDocumentMetadata;
}

export interface NotebookCell {
    readonly uri: Uri;
    readonly handle: number;
    readonly language: string;
    readonly cellKind: CellKind;
    readonly outputs: CellOutput[];
    readonly metadata?: NotebookCellMetadata;
    getContent(): string;
}
@roblourens
Copy link
Member

Does the edit API need to support reordering? Like could there any transient state in webview cell outputs that would not be represented in CellOutput?

@roblourens
Copy link
Member

roblourens commented Mar 24, 2020

If I set changes.metadata to something, am I overwriting all of metadata or merging in the properties that are set?

And I think metadata should be non-optional, even if all its properties are, it makes lots of things simpler.

@rebornix
Copy link
Member Author

Does the edit API need to support reordering?

Extensions already use splice to do reordering since Cell are created through NotebookEditor.createCell so Cells are managed objects. We need to make sure they are not disposed during splice.

Like could there any transient state in webview cell outputs that would not be represented in CellOutput?

This should probably be done by output in the webview sending its state through webview api to extensions.

If I set changes.metadata to something, am I overwriting all of metadata or merging in the properties that are set?

It should be override IMHO.

And I think metadata should be non-optional, even if all its properties are, it makes lots of things simpler.

I agree making metadata non-optional is clear to both us (for implementation) and extensions author (maybe) but it will make cell-default-metadata on document impossible, unless we use inherit | enable | disable enum instead of boolean.

@jrieken
Copy link
Member

jrieken commented Mar 24, 2020

We could make this even more similar to TextEditor#edit by also having an edit-callback which in the only place to create new cell, delete cell etc. Something like this

export interface NotebookEditorCellEdit {
	insert(content: string, language: string, type: CellKind, outputs: CellOutput[], metadata: NotebookCellMetadata | undefined): void;
	delete(cell: NotebookCell): void;
	// more
}

export interface NotebookEditor {
	readonly document: NotebookDocument;
	viewColumn?: ViewColumn;

	readonly onDidReceiveMessage: Event<any>;

	postMessage(message: any): Thenable<boolean>;

	editCells(callback: (cellEdit: NotebookEditorCellEdit) => any): any
}

@roblourens
Copy link
Member

I agree making metadata non-optional is clear to both us (for implementation) and extensions author (maybe) but it will make cell-default-metadata on document impossible, unless we use inherit | enable | disable enum instead of boolean.

Right, I mean having metadata non-optional but its properties can be optional, where undefined means "inherit"

@rebornix
Copy link
Member Author

Current state of the Notebook API: we added Document Edit API and make document.cells readonly. All changes to the cells should be performed through the Edit API

export interface NotebookEditorCellEdit {
    insert(index: number, content: string, language: string, type: CellKind, outputs: CellOutput[], metadata: NotebookCellMetadata | undefined): void;
    delete(index: number): void;
}

export interface NotebookEditor {
    readonly document: NotebookDocument;
    viewColumn?: ViewColumn;
    readonly onDidReceiveMessage: Event<any>;
    postMessage(message: any): Thenable<boolean>;
    edit(callback: (editBuilder: NotebookEditorCellEdit) => void): Thenable<boolean>;
}

The renderer API is also simplified as

export interface NotebookOutputRenderer {
    render(document: NotebookDocument, output: CellOutput, mimeType: string): string;
    preloads?: Uri[];
}

Note that we didn't pass NotebookEditor to renderers, instead they can only get the NotebookDocument. This is because The same document can be opened in two editors but the document is shared. My current thinking is custom renderers don't care about which editor the transformed output (html) will be rendered in and it also doesn't talk with the output through webview api because it doesn't know which editor the document will be loaded in.

@connor4312
Copy link
Member

connor4312 commented Mar 27, 2020

When implementing it the NotebookOutputRenderer felt a little awkward. The types indicated that I need check the output kind and check that rich output had data matching the mime type I care about... and throw an error if it's not right? Or return an empty string? Hard to know. It's also a little confusing to me because it seems like render will only be called for my own mime types, not CellStreamOutput/CellStreamInput. This was my boilerplate:

render(document: vscode.NotebookDocument, output: vscode.CellOutput): string {
  if (output.outputKind !== vscode.CellOutputKind.Rich) {
    return '';
  }

  if (output.data[Constants.TableMimeType] === undefined) {
    return '';
  }

  // actually do stuff...
}

Maybe the NotebookOutputRenderer's render function could be render(document: NotebookDocument, source: string, mimeType: string): CellOutput, where vscode would only call the renderer and pass in the source when the cell mime type matches. I also like the idea proposed of returning a CellOutputKind--gives me an easy way to deal with errors and higher-order renderers might be interesting.

@rebornix
Copy link
Member Author

rebornix commented Apr 7, 2020

@connor4312 thanks for the feedback. you are right that customs renderers should not check cellKind as we are invoking custom renderers only for CellDisplayOutput.

The API is changed to

export interface NotebookOutputRenderer {
  render(document: NotebookDocument, output: CellDisplayOutput, mimeType: string): string;
  preloads?: Uri[];
}

No functionality changes under the hood, just making the type safe. We can discuss about whether we should return CellDisplayOutput in next API meeting.

rebornix added a commit that referenced this issue Apr 7, 2020
@rebornix
Copy link
Member Author

rebornix commented Apr 11, 2020

Discussion points for coming API sync (April 14th)

  • NotebookCell.source should be string[] other than string. Firstly we are storing contents as string[] thus returning string means we need to do concatenation but it can be overhead as extensions might need to split the NotebookCell.source into individual lines again before saving to disk.
  • NotebookProvider.save(document: NotebookDocument): Promise<boolean>; should the first argument be editor: NotebookEditor just in case the notebook provider wants to talk to the webview?
  • Cell content update API
export interface NotebookEditorCellEdit {
    insert(index: number, content: string | string[], language: string, type: CellKind, outputs: CellOutput[], metadata: NotebookCellMetadata | undefined): void;
    delete(index: number): void;
+   replace(index: number, value: string): void;
}

@jrieken
Copy link
Member

jrieken commented Apr 14, 2020

NotebookCell.source should be string[] other than string. Firstly we are storing contents as string[] thus returning string means we need

Not a fan, we should not bleed implementation details into the API and there is no obvious reason why cells should be kept as lines. Also, this would be different from the text document content provider API which returns a string. If performance is of outmost matter here then we should consider talking bytes instead of strings

@sguillia
Copy link

I am wondering if you are planning to have all cells one below the other, just like in a notebook.

Or if we will be able to have one cell occupy 50% of the webview width, as customizable as an HTML textarea.

I believe extensions can benefit from having native editors and HTML content side-by-side, even if having native editors is already a great win.

@rebornix
Copy link
Member Author

@sguillia the current UX for the native notebook will be a list of cells, or in your words, one cell below the other.

@JeffreyCA
Copy link
Member

With the stable API, Is it currently possible to create a webview like the Interactive Playground, with Markdown interleaved with editable code areas?

@jrieken
Copy link
Member

jrieken commented May 27, 2021

Please be aware of the following BREAKING change: NotebookCellExecutionTask and its factory function has been renamed NotebookCellExecution and createNotebookCellExecution

@jrieken
Copy link
Member

jrieken commented May 27, 2021

Please be aware of the following BREAKING change: Support for the application/x.notebook.error-traceback MIME type has been removed, use vscode.NotebookCellOutput.err(...) instead

@jrieken
Copy link
Member

jrieken commented May 27, 2021

Please be aware of the following BREAKING change: NotebookCellOutputItem.value is gone. The data property must be used instead. The same is true for renderers and CellInfo

@jrieken
Copy link
Member

jrieken commented May 27, 2021

Please be aware of the following BREAKING change: NotebookKernelPreload has been renamed to NotebookRendererScript, also it's now NotebookController.rendererScripts

@jrieken
Copy link
Member

jrieken commented May 28, 2021

Please be aware of the following BREAKING changes

  • NotebookCellExecution.document has been removed, use NotebookCellExecution.cell.notebook instead
  • appendOutput | replaceOutput | clearOutput now accepts a cell instance instead of an index. (This change is backwards compatible, e.g there will be a compile error but no change in runtime behaviour)

@jrieken
Copy link
Member

jrieken commented May 28, 2021

Please be aware of the following "SEMI BREAKING" change: we have renamed NotebookCellOutput#outputs to NotebookCellOutput#items. The old property has been removed from the API file but is kept functional - we kept "binary compatibility" but not compile compatibility.

@jrieken
Copy link
Member

jrieken commented May 31, 2021

Please be aware of the following change: the namespace has been renamed to vscode.notebooks (plural). This change is not yet breaking but must be adopted ASAP.

@jrieken
Copy link
Member

jrieken commented May 31, 2021

Please be aware of the following changes: NotebookDocument#viewType has been renamed to NotebookDocument#notebookType, also NotebookController#viewType has been renamed to NotebookController#notebookType. Likewise, package.json contribution should be using type instead of viewType. These changes are not yet breaking but must be adopted by extension ASAP.

@jrieken
Copy link
Member

jrieken commented Jun 1, 2021

Please be aware of the following BREAKING changes

  • NotebookCellOutputItem#metadata has been removed, only NotebookCellOutput#metadata should be used
  • the special type definitions for NotebookDocumentMetadata and NotebookCellMetadata have been replaced with just { [key:string]: any}

@roblourens
Copy link
Member

BREAKING changes:

  • The constructor for NotebookCellStatusBarItem now only takes the required properties. You will have to set the other properties on the created object
  • The object parameters to NotebookCellExecution#start/end have been inlined to individual parameters
  • NotebookCellExecutionSummary#startTime/endTime have been combined to NotebookCellExecutionSummary#timing. vscode will not expose the invalid state where only one of them has been set.

@jrieken
Copy link
Member

jrieken commented Jun 2, 2021

Please be aware of the following BREAKING changes:

  • the notebookViewType context key has been renamed to notebookType
  • the deprecated properties viewType and hasExecutionOrder have been removed
  • the following symbols have been moved from the notebooks namespace to the workspace namespace: onDidCloseNotebookDocument, onDidOpenNotebookDocument, registerNotebookSerializer, openNotebookDocument, notebookDocuments
  • the ctor of NotebookCellData now only takes the required properties. You will have to set the other properties on the created object

@jrieken
Copy link
Member

jrieken commented Jun 2, 2021

Please be aware of the following BREAKING change: support for viewType is package.json has been dropped and type MUST BE used (related to #93265 (comment))

@jrieken
Copy link
Member

jrieken commented Jun 2, 2021

Please be aware of the following BREAKING change: the deprecated notebook namespace has been removed and notebooks (plural) must be used

@jrieken
Copy link
Member

jrieken commented Jun 3, 2021

Please be aware of the following BREAKING change: the onDidChangeNotebookAssociation has been renamed to onDidChangeSelectedNotebooks

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 3, 2021

BREAKING change for the new renderer api: #125351

Renames a few types and functions, and also switches around some argument orders

Working to update the published types and example extensions now

@jrieken
Copy link
Member

jrieken commented Jun 4, 2021

Please be aware of the following BREAKING change: we have inlines the NotebookExecuteHandler type. This is compile-only breakage and doesn't affect the execution

@jrieken
Copy link
Member

jrieken commented Jun 16, 2021

Please speak up if you are using the vscode.notebook.createConcatTextDocument()-API. No customer is known to us and it is scheduled for removal: #126485

@rchiodo
Copy link
Contributor

rchiodo commented Jun 16, 2021

Please speak up if you are using the vscode.notebook.createConcatTextDocument()-API. No customer is known to us and it is scheduled for removal: #126485

We use it in the python extension to support concating cells for intellisense.

Here exactly:
https://github.com/microsoft/vscode-python/blob/3698950c97982f31bb9dbfc19c4cd8308acda284/src/client/jupyter/languageserver/notebookConcatDocument.ts#L66

@jrieken
Copy link
Member

jrieken commented Jun 16, 2021

Ok, good to hear. In that case, I don't remove it ;-)

@connor4312
Copy link
Member

Note that the order of arguments in NotebookRendererMessaging.postMessage has changed. There is a back-compat patch in place for the moment:

90aa979#diff-f127724f8c5dbf0c8371ad0a100f8a9bc0a398b6b8ec29aa6cd7f265bd01a096R11580

@jrieken
Copy link
Member

jrieken commented Mar 16, 2022

🔈 events concerning the notebook documents are moving into its own, consolidated proposal: #144662 (comment). As of today both variants are supported but extensions are asked to migrate onto the notebookDocumentEvents-proposal ASAP

@jrieken
Copy link
Member

jrieken commented Mar 24, 2022

FYI that with 1.67(-insiders) the deprecated events from the notebookEditor-proposed are being removed. Instead the new notebookDocumentEvents proposal should be used

@DonJayamanne
Copy link
Contributor

@jrieken the new API has been adopted in the Jupyter extension

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 17, 2022

Closing this in favor since we've finalized a good chunk of the notebook API. Will keep following up the remaining notebook proposals in individual issues

@mjbvz mjbvz closed this as completed Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api notebook notebook-api plan-item VS Code - planned item for upcoming
Projects
None yet
Development

No branches or pull requests

13 participants