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.

Sign up for GitHub

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

Jump to bottom

Revisit webview.ts in workbench core #165889

Closed
bpasero opened this issue Nov 9, 2022 · 1 comment · Fixed by #166313
Closed

Revisit webview.ts in workbench core #165889

bpasero opened this issue Nov 9, 2022 · 1 comment · Fixed by #166313
Assignees
@mjbvz
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders webview Webview issues
Milestone
November 2022

Comments

@bpasero
Copy link
Member

bpasero commented Nov 9, 2022 •
edited
Loading

I was a bit surprised to find src/vs/workbench/browser/webview.ts and src/vs/workbench/common/webview.ts. As far as I know, webviews are a workbench contribution and not workbench core. I typically want the workbench core not provide things that only contrib depends on so I would recommend to move these files into src/vs/workbench/contrib/webview/ component.

This becomes especially interesting as soon as we explore to lazily load contrib (#164068)

If we have a dependency to anything in there from non-contrib then we can revisit that case by case, but I could not find any.
@bpasero bpasero added debt Code quality issues webview Webview issues labels Nov 9, 2022
@mjbvz
Copy link
Collaborator

mjbvz commented Nov 14, 2022

  • src/vs/workbench/browser/webview.ts: This file should likely just be renamed as it's used by webWorkerExtensionHost too and not webview specific

  • src/vs/workbench/common/webview.ts: This file is used in ext host code and in notebooks. I'll see about moving it under contrib/webview

@mjbvz mjbvz added this to the November 2022 milestone Nov 14, 2022
mjbvz added a commit to mjbvz/vscode that referenced this issue Nov 14, 2022
@mjbvz
Move and rename webview.ts files
d7037af 
Fixes microsoft#165889
@mjbvz mjbvz mentioned this issue Nov 14, 2022
Merged
mjbvz added a commit that referenced this issue Nov 14, 2022
@mjbvz
Move and rename webview.ts files (#166313)
57bceb6 
Fixes #165889
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Nov 14, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mjbvz mjbvz

Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders webview Webview issues
Projects
None yet
Milestone
November 2022
Development

Successfully merging a pull request may close this issue.

Move and rename webview.ts files mjbvz/vscode
3 participants
@bpasero @mjbvz @vscodenpa