feat(lsp): upgrade check on init and notification #21105
Conversation
nayeemrmn
changed the title
nayeemrmn
changed the title
cli/tools/upgrade.rs
Outdated
| pub async fn check_for_upgrades_for_lsp( | ||
| http_client: Arc<HttpClient>, | ||
| ) -> Result<Option<(String, bool)>, AnyError> { | ||
| let is_canary = version::is_canary(); | ||
| let latest_version; | ||
| let mut is_upgrade; | ||
| if is_canary { |
There was a problem hiding this comment.
This seems to be checking for new version unconditionally - we should reuse logic from check_for_upgrades that checks for DENO_NO_UPDATE_CHECK as well as checks only once every few hours. That will also allow us to unit test this functionality.
There was a problem hiding this comment.
The env var check is at the call site. We can't reuse the cooldown logic because we have to know if there is or isn't an update available every time the LSP is connected to so that status shown in the extension is accurate.
There was a problem hiding this comment.
Okay, but can we do it in a task that doesn't block the LSP from starting up? It appears currently this is done serially and the LSP waits for the update check to finish.
There was a problem hiding this comment.
It appears currently this is done serially and the LSP waits for the update check to finish.
Nope it's done in the initialized notification handler and outside of the LS lock. initialized is responseless, the client will treat the server as ready after just sending it.
There was a problem hiding this comment.
Maybe it would be good to update or add a test that uses a url on the test server that will cause a hang for say 20 seconds? Then the test could assert it took less than 20 seconds? It would help in case there's ever a regression.
There was a problem hiding this comment.
Seems like it's really complicated to embed test URLs in the LSP without a public API (e.g. NPM_CONFIG_REGISTRY). The LSP test harness for some reason (sometimes?) spawns a server with the non-test binary so conditional compilation is weird. Doesn't seem worth it
There was a problem hiding this comment.
The LSP test harness for some reason (sometimes?) spawns a server with the non-test binary so conditional compilation is weird.
Were you running cargo build after making changes to the test server?
There was a problem hiding this comment.
Were you running
cargo buildafter making changes to the test server?
I don't thinks so. Maybe it would work if I built test_util most recently with cargo test test_util, but... yeah. I think in the future we can make more of the language server centre unit testable.
Coupled with denoland/vscode_deno#988.
There's not much to unit test and an integration test isn't feasible. You can manually test by rolling back the version in
cli/Cargo.toml, building, launching post-patch vscode_deno against that binary and clicking the status bar.