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

Remove kernel preloads globals API #163513

Closed
mjbvz opened this issue Oct 13, 2022 · 5 comments · Fixed by #170909
Closed

Remove kernel preloads globals API #163513

mjbvz opened this issue Oct 13, 2022 · 5 comments · Fixed by #170909
Assignees
Labels
api debt Code quality issues notebook
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 13, 2022

Kernel preloads can currently be loaded two ways:

  • As a module that exports an activate function. We invoke this function with the notebook API
  • As a global script. In this case, we inject the notebook API as global variables

I would like to remove the second approach as it is not something we want to finalize. For now we can still allow global scripts, but they should not have access to the VS Code apis such as acquireVsCodeApi

@DonJayamanne @rebornix Can you please check to see if the jupyter preloads use the global versions of any of the following API:

  • acquireVsCodeApi
  • onDidReceiveKernelMessage
  • postKernelMessage

If you are using these, is it possible to switch to use module based preloads instead? If you cannot, what are the blockers?

@DonJayamanne
Copy link
Contributor

@mjbvz I've made the changes in Jupyter, will remote the corresponding VS code changes and test this out later today/tomorrow.

@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jan 10, 2023
@DonJayamanne
Copy link
Contributor

@mjbvz
Isn't the following code also meant to be removed?
src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts

If so, then this issue has not been completed and I've missed some other parts.

	async function activateModuleKernelPreload(url: string) {
		const module: KernelPreloadModule = await __import(url);
		if (!module.activate) {
			console.error(`Notebook preload '${url}' was expected to be a module but it does not export an 'activate' function`);
			return;
		}
		return module.activate(createKernelContext());
	}

@DonJayamanne DonJayamanne reopened this Jan 17, 2023
@VSCodeTriageBot VSCodeTriageBot removed the insiders-released Patch has been released in VS Code Insiders label Jan 17, 2023
@mjbvz
Copy link
Collaborator Author

mjbvz commented Jan 17, 2023

@DonJayamanne We can remove that once the stable version of Jupyter is also using modules. Has that release switched over or do we need to wait?

@DonJayamanne
Copy link
Contributor

Has that release switched over or do we need to wait?

Not yet, I missed one bit and here's the corresponding PR microsoft/vscode-jupyter-ipywidgets#16

@DonJayamanne
Copy link
Contributor

This should now be fixed.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api debt Code quality issues notebook
Projects
None yet
4 participants