-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update the vscode language server setup #4663
base: trunk
Are you sure you want to change the base?
Conversation
utils/vscode/esbuild.js
Outdated
*/ | ||
|
||
/** | ||
* This supports using esbuild to bundle the extension for releases. |
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.
This comment wasn't enough for me to figure out what this file is doing. Can you add some more description here? Specifically, it looks like this is a program that can be run using node
; what is its command-line syntax and what does it do when run?
It would also have helped me for this file to point at the invocation of this from the scripts in package.json
, and maybe to also point at the uses of the scripts in .vscode/tasks.json
.
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.
Linked back to https://code.visualstudio.com/api/working-with-extensions/bundling-extension
I'm not sure I'm getting what you're looking for exactly though, maybe you can suggest changes?
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.
To be clear, I'm mostly re-using code here. I didn't write this myself (and I'm not familiar with esbuild), so I don't have a strong grasp of it beyond that it fixes a warning from vsce.
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.
@chandlerc Updated file per discussion.
Co-authored-by: Richard Smith <[email protected]>
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.
Noticed prettier wasn't formatting json, so fixed that.
- If you're using VS Code locally, run | ||
`npm install && vsce package -o carbon.vsix && code --install-extension carbon.vsix` |
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.
FWIW, this command works for me using VS Code's remote mode too. You might need to run it from the VS Code terminal for code
to properly communicate with the running VS code instance.
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.
LGTM, leaving to @chandlerc to approve once he's happy.
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.
Looks amazing to me TBH! Excited about this.
Switches from js to ts, and starts bundling files in order to produce a better package for deployment. Fixes the README.md to be a more appropriate front page, moving dev content to development.md. Makes the path to
carbon
configurable so that it's more stable than just running inbazel-bin
.This is built using suggestions from samples at https://github.com/microsoft/vscode-extension-samples/tree/main/lsp-sample and https://github.com/microsoft/vscode-extension-samples/tree/main/esbuild-sample. Note the esbuild in particular comes from complaints from
vsce
to use an option from https://code.visualstudio.com/api/working-with-extensions/bundling-extension, and esbuild is just the first option detailed there (I have no real opinion on options).I'm bumping the version, and will do a release after merging.