search

microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.65k stars 28.68k forks source link

Split up workbench/api into a node and browser part #70319

Closed bpasero closed 5 years ago

bpasero commented 5 years ago

For https://github.com/Microsoft/vscode/issues/68302

We will eventually have to move our vs/workbench/api/electron-browser/mainThread.* files into browser or common to support running extensions without node integration.

When I look at an example (mainThreadLanguages.ts) a common pattern seems to be a dependency to node/extHost.protocol which seems to contain both types for the renderer side as well as the extension host side.

As a first step I would think node/extHost.protocol needs to split up into 2 parts, where the one is clearly the types accessed within the extension host and the other from the renderer side (which then can live in common or browser).

Thoughts?

jrieken commented 5 years ago

As a first step I would think node/extHost.protocol needs to split up into 2 parts, where the one is clearly the types accessed within the extension host and

I believe that extHost.protocol is pretty independent of node-api and it it can be moved to common, at least I would aim for that.

bpasero commented 5 years ago

@jrieken when I look at extHostProtocol it seems to reach into extHostTypes which eventually requires vscode. Otherwise, there are indeed not many dependencies to node/electron-browser it seems.

jrieken commented 5 years ago

The vscode module is always there

bpasero commented 5 years ago

@jrieken does it make sense to require vscode from the renderer though ever? Imho that should only be depended on from the extension host.

jrieken commented 5 years ago

Yes and No, in extHostTypes and other places that implement the API it does make sense, esp. with interfaces that we don't want to duplicate everywhere.

jrieken commented 5 years ago

⬆️ The list above is for electron 5 readiness. We need to move mainThreadXYZ files to the api/browser layer which means that they don't depend on node-api.

jrieken commented 5 years ago

re https://github.com/Microsoft/vscode/issues/70319#issuecomment-472366964 - I have changed my mind, we shouldn't leak the vscode-module into our "normal" sources, e.g. outside the extension host. I have copied a couple of interfaces into extHost.protocol.ts and when moving things to browser you might have to do the same.

bpasero commented 5 years ago

The list is down to:

alexr00 commented 5 years ago

Thanks @bpasero for moving mainThreadtTask.ts!