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 renderer: state persistence #95361

Closed
rebornix opened this issue Apr 15, 2020 · 13 comments
Closed

Notebook renderer: state persistence #95361

rebornix opened this issue Apr 15, 2020 · 13 comments
Assignees
Labels
feature-request Request for new features or functionality notebook on-testplan polish Cleanup and polish issue
Milestone

Comments

@rebornix
Copy link
Member

Issue Type: Feature Request

When building a regular webview extenison or a custom editor, extension authors can follow the existing guidance https://code.visualstudio.com/api/extension-guides/webview#persistence for intermediate state persistence. Extensions will use the vscodeAPI to communicate with the extension and also get/set its intermediate state for recovery. The API is as below

const vscode = acquireVsCodeApi();

// Check if we have an old state to restore from
const previousState = vscode.getState();
...

setInterval(() => {
  vscode.setState({ count });
}, 100);

acquireVsCodeAPI can only be run once, which ensures that it's not being used unexpectedly. However in Notebook world, there are multiple layers who want to access this API

  • Our core logic. It uses this API to send out scroll and sizeChange events.
  • Renderers. We allow multiple renderers to be loaded in the same output webview.

To allow renderers to communite with its coresponding extension, we relaxed it a bit to allow acquireVSCodeAPI to be run multiple times. However it's a bad idea as renderers now don't know how to save/restore states correctly, as vscode.setState can be run by any code.

One solution is creating vscodeAPI object for every indidual player

const vscode = acquireVsCodeApi('_vscodeCore');
const ipyWidgets = acquireVsCodeApi('python.ipywidget');

vscode.getState();
...
ipyWidgets.getState();

acquireVsCodeApi with the same id can only be invoked once. This way each renderer can have its own communication channel with its extension and also its own state restoration.

Particularly for ipywidgets, it should always setState before it sends states to the kernel side and read state when it's being restored.

cc @kieferrm @mjbvz @connor4312

VS Code version: Code - Insiders 1.45.0-insider (44c5185, 2020-04-10T19:43:46.226Z)
OS version: Darwin x64 19.2.0

@rebornix rebornix added this to the April 2020 milestone Apr 15, 2020
@rebornix rebornix added the polish Cleanup and polish issue label Apr 15, 2020
@connor4312
Copy link
Member

connor4312 commented Apr 15, 2020

This works. In some cases I think consumers may prefer to acquire the API based on the cell URI if they are building them as self-contained 'apps'. For instance in the flame graph extension I would want to store the view position on a per-graph level and don't need an extension-level state. This API would not prevent renderers from doing that.

@rebornix
Copy link
Member Author

@connor4312 makes sense, renderers can build an id from the cell uri. To archive that, the renderer needs to read the uri from the cell when it converts CellDisplayOutput to HTML fragment.

render(document: NotebookDocument, output: CellDisplayOutput, mimeType: string): string {
  const id = output.uri.toString();
  ...
}

@mjbvz
Copy link
Collaborator

mjbvz commented Apr 15, 2020

@rebornix I believe your proposal to use acquireVsCodeApi(id: string) should work. I'd be even nicer if vscode could export a scoped version of acquireVsCodeApi to each cell automatically however I'm not sure that's compatible with how notebooks work

One other point about the api proposal: we may need a way to clean up the state of cells that no longer exist

@connor4312
Copy link
Member

I'd be even nicer if vscode could export a scoped version of acquireVsCodeApi to each cell automatically however I'm not sure that's compatible with how notebooks work

I'm not sure how this could be done safely (allowing for async calls) without something like Zone.js, which can be a difficult can of worms to deal with. I'll look into this some more when I do the work renderer iteration but I think string-scoped APIs are a good basis to work on.

@connor4312
Copy link
Member

  • Something that came up before is the idea that we should probably not call it acquireVsCodeApi in the context of notebook renderers, since they will also be usable in places like ADS. Also, using the same name as the webview API overloads the term in the event that we add any extra properties for renderers.
  • Thinking of states as scopes. I think the greatest scope that a renderer should be able to access is document -> renderer. Generally renderers will be independent and allowing renderers to share a global state will undoubtedly lead to problems if users install bad-behaving renderers.
  • As mentioned, in many cases the cells may be independent from each other, so having a way to get the per-cell state is good.
  • I don't think there's a reason to require these to be separate acquireVsCodeApi calls just for different states, since methods like (and currently only) postMessage is the same between all of them.

This leads me to an API something like this:

interface IState<T> {
  retrieve(): T | undefined;
  retrieve(defaultValue: T): T;
  store(state: T): Promise<void>; // or void? technically the postMessage to store is async
}

interface IRendererApi<CellState, NotebookState> {
  cellState: IState<CellState>; // per-cell
  notebookState: IState<NotebookState>; // per-renderer in the notebook
  postMessage(msg: unknown): void;
}

declare function acquireNotebookRendererApi<CellState = any, NotebookState = any>():
  IRendererApi<CellState, NotebookState>;

Q: is the cell state cleared when the user re-evaluates the cell? It could be useful if the user is, for example, zoomed in on a graph.

The caveat is that acquireNotebookRendererApi() alone won't work -- we need some way to identify the current cell when that's being called. That could be the document.currentScript, which would work if cells get evaluated like how I have them in the stater. But if, for example, a renderer renders output by attaching a MutationObserver and running code when a certain element is created, that won't work.

@connor4312
Copy link
Member

connor4312 commented May 19, 2020

Followup from call: cell state is probably unnecessary given two things:

  • We can identify cell outputs. Currently I think the right way to do this in NotebookOutputRenderer.render is to use document.cells.find(cell => cell.outputs.includes(output))?.uri. Maybe this can be exposed more easily, @rebornix for thoughts
  • We can know when cells get unrendered. I think the best way to do this is an event exposed on the API which is called immediately before the cell gets removed from the DOM.

That would make the API something like this:

interface IRendererApi<State> {
// mostly-existing on the webview api:
  setState(value: T): void;
  getState(): T | undefined;
  getState(defaultValue: T): T | undefined;
  postMessage(msg: unknown): void;
  
// new: 
  readonly onCellWillUnmount: Event<string /* cell uri */>;
}

declare function acquireNotebookRendererApi<State = any>(): IRendererApi<State>;
  • This will require acquireNotebookRendererApi to be called synchronously in the preload script, I think, so we can associate it with a specific renderer.

Q:

  • Should we also have React-like onCellDidMount: Event<string /* cell uri */, HTMLElement>? May be a convenient alternative to my onload hook
  • Have a separate onCellWillRerender if we know that we're going to immediately recreate the output?
  • The EventEmitter/Event is in the vscode core. Can we easily bundle it for webviews? Or should we have a minimal copy of the implementation in the webview code?

@roblourens
Copy link
Member

This will require acquireNotebookRendererApi to be called synchronously in the preload script, I think, so we can associate it with a specific renderer.

I don't think that's required, we can wrap the renderer scripts in an IIFE that provides the right function under that name

Or should we have a minimal copy of the implementation in the webview code?

I think it's better to reimplement this stuff since otherwise you'll pull in all of vs/base/common

@connor4312
Copy link
Member

connor4312 commented May 26, 2020

I don't think that's required, we can wrap the renderer scripts in an IIFE that provides the right function under that name

To do this we need to intercept the script. We might be able to do that for our own URIs but in development the files can load from http://localhost. It also would not make them available to anything outside the file, like hot-module-loaded code or code-split paths.

One way might be to allow users to call acquireNotebookRendererApi synchronously or with their renderer ID.

@connor4312
Copy link
Member

connor4312 commented May 26, 2020

Initial implementation:

  • branch: https://github.com/microsoft/vscode/compare/connor4312/webview-state-api

  • I have a branch of the stater with .d.ts and some usage of the new APIs: https://github.com/microsoft/vscode-notebook-renderer-starter/compare/enhanced-api

    In particular, I found onload in my script stopped working at some point (and the internet suggests this shouldn't have ever worked for a script without an src...) Earlier I had put in a workaround but the onDidMountCell API provides a graceful workaround for the issue.

    • We should make the cell URI more accessible to the renderer and/or output. Without that I can't currently do any cell-level APIs.
    • At the moment I just tack on state management to the base vscode API. As mentioned in standup, the presence of this state brings up the question around whether it's something that content providers would be interested in serializing in the notebook.

Other observations:

  • Debugging webview internal code is a pain
  • Not having types also makes writing this painful (currently all webview side code, including JS parts, is in an HTML template literal), maybe @eamodio's work with getting webpack bundles in our build process will help here? Ideally we could write the notebook API in a standalone TS file and then get it as a string in the editor code.
  • Not sure how to unit test this code. Maybe can be fixed with a bundling solution...

@jrieken
Copy link
Member

jrieken commented May 27, 2020

Should we also have React-like onCellDidMount: Event<string /* cell uri */, HTMLElement>? May be a convenient alternative to my onload hook

I like those ideas but I wouldn't use react lingo but something that's more familiar in pure html terms

@rebornix
Copy link
Member Author

First of all, good work and I like the shape of the API in overall.

One way might be to allow users to call acquireNotebookRendererApi synchronously or with their renderer ID.

Not a fan of using document.currentScript as the namespace. Like you said above, it might not work if renderers use split bundles and it also makes it unclear what's the expected behavior when renderers generate inline scripts in outputs (as the namespace can change). acquireNotebookRendererApi(renderType) has no such problem if I understand correctly.

onDidMountCell

Currently we support rendering multiple outputs for a single cell (we might still have some layout issue with this) so we may need onDidMountCellOutput other than onDidMountCell. Also a cell can have multiple outputs rendered by different renderers.

onWillUnmount* / onDidUnmount*

Similarly, we need onWillUnmountOutput but the catch is we don't have identifiers for outputs, so we can't describe onDidUnmount* as the DOM element is then already removed from the DOM tree. Probably we can have a single event onBeforeUnmount*: Event<Uri, HTMLElement>

Not having types also makes writing this painful (currently all webview side code, including JS parts, is in an HTML template literal), maybe @eamodio's work with getting webpack bundles in our build process will help here?

The challenge is we need to inline the scripts in the HTML template as Webview iframes are hosted in another domain.

Ideally we could write the notebook API in a standalone TS file and then get it as a string in the editor code.

This can work and we did that for vs/loader.js https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts#L190-L205 .

@connor4312
Copy link
Member

Thanks for the feedback! Updated my PR:

  • I moved the inline script into a separate file, which is TS, and I then stringify the function to inject it into the HTML. Not the prettiest, but adds no overhead and stringification behavior of functions in js is ratified in ECMA. As part of this I took the opportunity to type the messages messages passed back and forth better.
  • Made the rendererType required.
  • Also, the event is onDidCreateCell and onWillDestroyCell. At the time this is called the cell will be in the DOM. These points match React's lifecycle hooks which in my experience have been good opportunities. We could have a "will create" and "did destroy" as well but this might be too extraneous.
  • For multiple cell outputs: I think we will need a way to identify outputs. What's the best way to do this? I could do something like cellUri#outputIndex?

@connor4312
Copy link
Member

Closing with the merged PR. We can continue discussion and iteration in a clean issue for next month.

@connor4312 connor4312 added feature-request Request for new features or functionality on-testplan and removed debt Code quality issues labels Jun 2, 2020
@connor4312 connor4312 modified the milestones: June 2020, May 2020 Jun 2, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality notebook on-testplan polish Cleanup and polish issue
Projects
None yet
Development

No branches or pull requests

5 participants