-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
editor/code: Allow to apply static analysis for codes opened in webview #15256
editor/code: Allow to apply static analysis for codes opened in webview #15256
Conversation
4ef41cb
to
da1755e
Compare
I rebased on the latest |
☔ The latest upstream changes (presumably #15265) made this pull request unmergeable. Please resolve the merge conflicts. |
da1755e
to
5c73223
Compare
5c73223
to
80ac389
Compare
entryPoints, | ||
format: "esm", | ||
platform: "browser", | ||
// VSCode v1.78 (Electron 22) uses Chromium 108. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're back on 1.75 since #15333.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- v1.78 updates to Electron 22.
- Before v1.78, vscode uses Electron 19 (Chromium 102.0.5005.167 and Node.js 16.17.1).
I'll update to:
- // VSCode v1.78 (Electron 22) uses Chromium 108.
+ // VSCode v1.75 (Electron 19) uses Chromium 102.
That's pretty bad, but I don't know if it will become a problem in practice. On the other hand, being able to move out all that code is nice. |
I think this is fine for now, we don't use a lot of webviews, nor do we much with them. |
I read that a bit differently -- being able to touch (at compile-, not run-time) the browser DOM from the main extension code. But I might be wrong. |
Yes. By the current design, we can touch DOM objects that does not exist in the vscode extension scope at the compile-time (at type level). For example, the current design does not prevent that VSCode's intellisense suggests To prevent fragile, I think we may take an approach to use npm workspace and typescript's project reference to tell a workspace dependency to typescript compiler. npm script cannot run it's with workspace dependency topological order npm/rfcs#548. Thus I choose typescript's project reference because I don't think it's a bit complex to introduce a monorepo tools nx or turborepo to this repository that already contains 2 programming language in heterogeneously. But this requires the change directory hierarchy and it would be major surgery. |
Can we run |
TypeScript's project reference automatically runs |
I'm indifferent to this, I have no typescript tooling knowledge so I can't really judgbe, but I'm fine with merging this since it looks beneficial. |
☔ The latest upstream changes (presumably #15728) made this pull request unmergeable. Please resolve the merge conflicts. |
Having given this another look,
does seem rather bad. So I'm more inclined to not merge this now after all |
Okay, I withdraw this pr once. Thank you for discussions |
This pull request introduces build phase to bundle a code opened in a webview by esbuild.
By this change, we are able to apply a static analysis for codes that are opened in webview.
As a caveat, we might confuse the execution context between vscode extension and webview after this change. For example, you may be able to touch DOM that is not actually in the vscode extension context but TypeScript does not warn it. I think this would not be a problem in a almost case by pushing codes into
src/webview/
directly.To prevents this problem, we need introduce a separated npm workspace to use an another tsconfig.json. But I think we don't have to do it in this moment. We should postpone it to the future.