Skip to content

Remove cycle browserHostService.ts -> web.api.ts.#198221

Merged
bpasero merged 3 commits into
microsoft:mainfrom
rehmsen:workspace_provider_cycle
Nov 15, 2023
Merged

Remove cycle browserHostService.ts -> web.api.ts.#198221
bpasero merged 3 commits into
microsoft:mainfrom
rehmsen:workspace_provider_cycle

Conversation

@rehmsen
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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
@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