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 cycle browserHostService.ts -> web.api.ts. #198221

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

rehmsen
Copy link
Contributor

@rehmsen rehmsen commented Nov 14, 2023

Before this change, IWorkbenchConstructionOptions in web.api.ts depended on IWorkspaceProvider in browserHostService.ts, which depends on some types from web.api.ts.

While this apparently does not pose problems in VS Code's build, other build system of downstream projects do not allow this, and it is also overall harder to follow the code if "the API" depends on types defined in ""the implementation".

Fixes #198220.

Before this change, `IWorkbenchConstructionOptions` in web.api.ts
depended on `IWorkspaceProvider` in browserHostService.ts, which
depends on some types from web.api.ts.

While this apparently does not pose problems in VS Code's build, other
build system of downstream projects do not allow this, and it is also
overall harder to follow the code if "the API" depends on types defined
in ""the implementation".
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this is fine to go in. There are more tangles with the web API (see #142646) but its unlikely I would pursue this any further because it does not cause issues in our build system.

@bpasero bpasero added this to the November 2023 milestone Nov 15, 2023
@bpasero bpasero merged commit b3a649f into microsoft:main Nov 15, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency cycle between browserHostService.ts and web.api.ts
5 participants