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

Environment service debt between common, browser, native #102918

Closed
inspirer opened this issue Jul 20, 2020 · 5 comments
Closed

Environment service debt between common, browser, native #102918

inspirer opened this issue Jul 20, 2020 · 5 comments
Assignees
Labels
debt Code quality issues *out-of-scope Posted issue is not in scope of VS Code

Comments

@inspirer
Copy link
Contributor

There are common and browser versions of the environment service but both of them depend on workbench.web.api.ts for IWorkbenchConstructionOptions. This is a type only dependency so it should not create any problems at runtime but it is misleading since common code gets access to browser-only interfaces such as IUpdateProvider.

The culprit seems to be the unclear status of workbench.web.api inside .eslintrc.json.

See https://github.com/microsoft/vscode/wiki/Source-Code-Organization#target-environments

@bpasero
Copy link
Member

bpasero commented Jul 20, 2020

Yeah, lot's of debt buried inside environment service, but I would hold on changing this until we figured out the sandbox model (#92164). I expect that the environment service will change a lot after this and I feel there is a chance to cleanup everything when we have an idea what this thing ends up to be.

If we find out that the web API options are only used in the web version, then maybe they should be removed from the common environment service and only be accessible from the browser workbench environment service. I am not keen on moving the options into core though out of workbench.web.api.ts because I like to track these interfaces inside one file ideally. But again, haven't spend a lot of though on it yet.

@bpasero bpasero changed the title common code depends on browser via environmentService breaking the allowed dependencies rules Environment service debt between common, browser, native Jul 20, 2020
@bpasero bpasero added the sandbox Running VSCode in a node-free environment label Jul 20, 2020
@inspirer
Copy link
Contributor Author

Sounds good, it is great to hear that this is being taken care of. Let's park (or drop) this PR for now.

The problem with keeping the options inside workbench.web.api.ts is that it makes that file a node in the imports graph that connects everything with everything (many services depend on it via the env service, and it depends on everything via workbench.web.main). The incremental compilation speed does not seem to suffer much as of now, so it is more of a potential threat of people keep adding more code and converting the env->web.api dependency into a proper dependency, turning half of workbench into a interconnected build component.

@bpasero bpasero removed the sandbox Running VSCode in a node-free environment label Sep 4, 2020
@bpasero
Copy link
Member

bpasero commented Sep 23, 2020

From #76719: we carry around multiple properties for backups (backupPath, backupHome, backupWorkspacesPath, backupWorkspaceHome) that should be consolidated.

@bpasero
Copy link
Member

bpasero commented Oct 12, 2020

As one step to untangle the environment service monster, I introduced a IMainEnvironmentService to have only those properties that are used only in electron-main. I was able to move the backup related main properties over, as well as some others only used there.

//fyi @sandy081

@bpasero bpasero added the *out-of-scope Posted issue is not in scope of VS Code label Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality issues *out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@inspirer @bpasero and others