-
Notifications
You must be signed in to change notification settings - Fork 147
Modernize #664
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
Modernize #664
Conversation
46f3ab7 to
b14fc62
Compare
|
@HighCommander4 I've updated this now that @clangd/install 0.1.19 is released. |
HighCommander4
left a comment
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.
Thanks for the patch!
| "icon": "icon.png", | ||
| "engines": { | ||
| "vscode": "^1.65.0" | ||
| "vscode": "^1.75.0" |
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.
Could you specify which change(s) in this patch necessitate this bump of the minimum required vscode version?
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.
I don't remember exactly :(
package.json
Outdated
| "vscode:prepublish": "npm run prepare && npm run esbuild -- --minify --keep-names", | ||
| "compile": "npm run esbuild -- --sourcemap", | ||
| "check-ts": "tsc -noEmit -p ./", | ||
| "prepare": "tsc -p ./", |
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.
Why remove -noEmit? The emission of javascript is done by esbuild, so now we're doing it twice in prepublish.
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.
Don't remember! Will revert.
| } | ||
| getState(): vscodelc.FeatureState { return {kind: 'static'}; } | ||
| dispose() {} | ||
| clear() {} |
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.
Adding some context for my own reference / posterity: vscode-languageclient renamed the dispose() method of the StaticFeature interface to clear() in microsoft/vscode-languageserver-node#1298.
I couldn't find an explanation of why; if you happen to know, I'd be curious.
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.
My guess is that dispose has other meaning: https://github.com/tc39/proposal-explicit-resource-management
package.json
Outdated
| "@clangd/install": "0.1.17", | ||
| "abort-controller": "^3.0.0", | ||
| "vscode-languageclient": "8.0.2" | ||
| "@clangd/install": "0.1.19", |
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.
I guess we need to do a new node-clangd release to pick up clangd/node-clangd@d567269 first.
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.
Ah, I guess so. I'll bump this to 0.1.20.
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.
Er, let me know when you've released and I'll update.
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.
Posted clangd/node-clangd#37
| }); | ||
| return Promise.resolve(result); // Thenable to real promise. | ||
| } | ||
| localize(message: string, ...args: Array<string|number|boolean>): string { |
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.
Ah, nice! I forgot about this. Looks like the person who contributed this support to node-clangd only cared about having it in coc-clangd 😆
This also partially answers my question about the vscode engine version: per the original discussion at #560, localization requires 1.73.
(You can add "Fixes #560" to the commit message.)
3d509f7 to
0344527
Compare
|
@HighCommander4 have a look at the CI failure. Seems the great minds behind unzipper decided to add a dynamic require in ZJONSSON/node-unzipper#325 for the AWS SDK for some reason, which is inherently incompatible with esbuild. |
15798a0 to
f540978
Compare
|
Ah, I was able to mark it external and that seems to work. |
package.json
Outdated
| "@types/mocha": "^10.0.7", | ||
| "@types/node": "^22.0.2", | ||
| "@types/sinon": "^17.0.3", | ||
| "@types/vscode": "1.97.0", |
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 seems wrong. I think this needs to match engines.vscode, because when I try to run npm run package locally, I get:
ERROR @types/vscode 1.97.0 greater than engines.vscode ^1.75.0. Either upgrade engines.vscode or use an older @types/vscode version
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.
Matched the engine version.
- Remove obsolete dependencies.
- abort-controller; `AbortController` is stable since Node 15.4.0.
- Update dependencies.
- @clangd/install; required as `AbortController` is part of the API
surface so be the same type.
Fixes clangd#560.
These are not required since VSCode 1.74.0. See https://code.visualstudio.com/api/references/activation-events#onCommand.
HighCommander4
left a comment
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.
Thanks, LGTM! CI is green and a local build is successful as well -- I'll go ahead and merge.
| "main": "./out/bundle", | ||
| "scripts": { | ||
| "esbuild": "esbuild ./src/extension.ts --bundle --outfile=out/bundle.js --external:vscode --format=cjs --platform=node", | ||
| "esbuild": "esbuild ./src/extension.ts --bundle --outfile=out/bundle.js --external:@aws-sdk/client-s3 --external:vscode --format=cjs --platform=node", |
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.
why do we need aws-sdk dependency here?
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.
I believe this is saying that we don't bundle this package. I don't remember why I had to add this flag. Try removing it and see?
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.
sent out #815
* Modernize
- Remove obsolete dependencies.
- abort-controller; `AbortController` is stable since Node 15.4.0.
- Update dependencies.
- @clangd/install; required as `AbortController` is part of the API
surface so be the same type.
Fixes clangd#560.
* Remove redundant activation events
These are not required since VSCode 1.74.0. See
https://code.visualstudio.com/api/references/activation-events#onCommand.
AbortControlleris stable since Node 15.4.0.Depends on clangd/node-clangd#32.