userpreferences parsing/ls config handing #1729
Conversation
iisaduan
requested review from
andrewbranch,
Copilot and
gabritto
and removed request for
andrewbranch and
Copilot
What is needed? I would think we would just ask for the setting blocks we are interested in and watch them, from the server side (no extension code needed). |
|
Our The other extension changes include to make configuration happen on initialization |
|
I feel confused. The LSP config spec is exactly the same as asking VS Code for a setting out of JSON. Putting more code in the client is just going to make that same code repeat across editors... |
internal/lsp/server.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| s.session.Configure(userPreferences) |
There was a problem hiding this comment.
Don't we also need to register with the client here to be able to actually get config change notifications? Just like we do in WatchFiles.
There was a problem hiding this comment.
Re: this + the comment above (link), I didn't fully implement this function because I wanted to figure out what we're going to process in the extension first and I wasn't sure the range of how much info that the client can pass to the server (will it always pass the entire new config upon changes? or only the differences?)
|
|
||
| // currently, the only request that may be sent by the server during a client request is one `config` request | ||
| // !!! remove if `config` is handled in initialization and there are no other server-initiated requests | ||
| if resp.Kind == lsproto.MessageKindRequest { |
There was a problem hiding this comment.
I do think we should move this handling to initialization. Or are going to need to handle other requests as well?
There was a problem hiding this comment.
I'm not sure what we will need in the future
There was a problem hiding this comment.
I think you could imagine the fourslash client handling diagnostics refresh requests and potentially even watch requests.
There was a problem hiding this comment.
If we're going to port the fourslash/server tests, then those probably will have to be handled?
There was a problem hiding this comment.
Pull Request Overview
This PR implements user preferences support throughout the codebase to unblock future work, focusing on fourslash test capabilities and inlay hints configuration from VS Code. The implementation adds user preferences handling to the session lifecycle, language service, and LSP server with initial support for parsing inlay hints settings from VS Code client configuration.
- Adds user preferences threading through session, snapshot, and language service layers
- Implements VS Code configuration parsing for inlay hints preferences
- Extends fourslash testing framework to support user preferences configuration
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/project/snapshot.go | Adds user preferences field and getter method to snapshot |
| internal/project/session.go | Implements user preferences configuration and change tracking in session |
| internal/project/api.go | Updates API calls to include user preferences parameter |
| internal/lsp/server.go | Adds configuration request handling and inlay hints parsing from VS Code |
| internal/ls/types.go | Extends UserPreferences with inlay hints fields and copy methods |
| internal/ls/languageservice.go | Adds user preferences parameter to language service constructor |
| internal/fourslash/fourslash.go | Implements user preferences configuration support for testing |
| internal/fourslash/tests/autoImportCompletion_test.go | Adds test case for user preferences functionality |
| internal/api/api.go | Updates language service instantiation to include user preferences |
…o userpreferences
…o userpreferences
| } | ||
| s.session.Configure(userPreferences) | ||
| } else { | ||
| // !!! handle userPreferences from initialization |
There was a problem hiding this comment.
When will we ever have these?
There was a problem hiding this comment.
requires changes in the extension, which I haven't done yet
There was a problem hiding this comment.
What extension? Us? The VS Code extension will always be able to request via the normal means, so I don't think we would include that info anywhere.
There was a problem hiding this comment.
theoretically, it would be nice to have userpreferences (and other preferences/config info) passed over in the actual initialization request through initializeParams.initializationOptions, so we don't have to wait for a configuration request to come back. It doesn't seem to be included by default, so we'd have to fix the extension to give extra info in initializationOptions
There was a problem hiding this comment.
I suppose so, but I've never seen a server which did that, honestly... It just means copying the same "get all of the options" code into the client another time, and then it gets copied into other clients, to save one roundtrip.
There was a problem hiding this comment.
oh huh it looks like the typescript-language-server already allows this https://github.com/typescript-language-server/typescript-language-server/blob/master/docs/configuration.md/#initializationoptions
There was a problem hiding this comment.
Yes, iirc because tsserver only really accepts prefs at init time, or supplied along with a request, as tsserver has no ability to call back to the client.
| registerRequestHandler(handlers, lsproto.ShutdownInfo, (*Server).handleShutdown) | ||
| registerNotificationHandler(handlers, lsproto.ExitInfo, (*Server).handleExit) | ||
|
|
||
| registerNotificationHandler(handlers, lsproto.WorkspaceDidChangeConfigurationInfo, (*Server).handleDidChangeWorkspaceConfiguration) |
There was a problem hiding this comment.
This handler is set up, but I don't think the PR sets up the watch capability yet.
iisaduan
changed the title
| result, err := s.sendRequest(ctx, lsproto.MethodWorkspaceConfiguration, &lsproto.ConfigurationParams{ | ||
| Items: []*lsproto.ConfigurationItem{ | ||
| { | ||
| Section: ptrTo("typescript"), |
There was a problem hiding this comment.
Unfortunately there is also javascript and js/ts scopes we might need.
I think we're going to have to make user prefs have both variants and then return one or the other depending on the situation...
There was a problem hiding this comment.
Not blocking for this PR, but needs to be done eventually.
| } | ||
|
|
||
| func (s *Session) UserPreferences() *ls.UserPreferences { | ||
| return s.userPreferences |
There was a problem hiding this comment.
This should be protected by a mutex (you can reuse s.pendingConfigChangesMu, maybe with a rename and change to a RWMutex)
s.compilerOptionsForInferredProjects would be in the same boat except that it’s literally never read 🤔
| // It should only be set the value in the next snapshot should be changed. If nil, the | ||
| // value from the previous snapshot will be copied to the new snapshot. | ||
| compilerOptionsForInferredProjects *core.CompilerOptions | ||
| newConfig *ls.UserPreferences |
There was a problem hiding this comment.
I don’t love the mix of terminology. Can we either standardize everything to use LSP terminology (workspace configuration) or say, once the “workspace configuration” has been filtered and parsed into stuff the language service cares about, we refer to that bit consistently as user preferences?
There was a problem hiding this comment.
I would prefer the latter, as in "try to resolve settings to a clearer set of shared options and use that".
4 participants
typescript.preferences, or a flat json like typescript-language-servertodo:
javascriptformat