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

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

Closed
bpasero opened this issue Mar 12, 2019 · 9 comments · Fixed by #70939
Closed

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

bpasero opened this issue Mar 12, 2019 · 9 comments · Fixed by #70939
Assignees
Labels
debt Code quality issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Mar 12, 2019

For #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
Copy link
Member

jrieken commented Mar 13, 2019

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
Copy link
Member Author

bpasero commented Mar 13, 2019

@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
Copy link
Member

jrieken commented Mar 13, 2019

The vscode module is always there

@bpasero
Copy link
Member Author

bpasero commented Mar 13, 2019

@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
Copy link
Member

jrieken commented Mar 13, 2019

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.

@bpasero bpasero changed the title Split up workbench/api into a electron-browser and browser part Split up workbench/api into a node and browser part Mar 14, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
@jrieken
Copy link
Member

jrieken commented Mar 19, 2019

⬆️ 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
Copy link
Member

jrieken commented Mar 19, 2019

re #70319 (comment) - 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 bpasero added this to the March 2019 milestone Mar 19, 2019
@weinand weinand removed their assignment Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 19, 2019
jrieken added a commit that referenced this issue Mar 20, 2019
jrieken added a commit that referenced this issue Mar 20, 2019
jrieken added a commit that referenced this issue Mar 20, 2019
jrieken added a commit that referenced this issue Mar 20, 2019
jrieken added a commit that referenced this issue Mar 20, 2019
@bpasero bpasero assigned mjbvz and unassigned bpasero, jrieken and alexdima Mar 22, 2019
@bpasero
Copy link
Member Author

bpasero commented Mar 22, 2019

The list is down to:

@alexr00
Copy link
Member

alexr00 commented Mar 22, 2019

Thanks @bpasero for moving mainThreadtTask.ts!

@alexr00 alexr00 removed their assignment Mar 22, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants