userpreferences parsing/ls config handing #1729
Conversation
iisaduan
requested review from
Copilot,
andrewbranch and
gabritto
and removed request for
Copilot and
andrewbranch
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
| } else if item, ok := item.(*UserPreferences); ok { | ||
| // case for fourslash | ||
| return item.CopyOrDefault() |
There was a problem hiding this comment.
I think we should avoid this, because it means we are not testing the parsing end to end at all. But, I assume the alternative means somehow converting UserPreferences back into LSP style objects?
There was a problem hiding this comment.
yeah. I did it this way because it feels easier to write the convert fourslash script to output UserPreferences struct, but it's also easy to add a case sending over map[string]any to do parsing testing
There was a problem hiding this comment.
Using UserPreferences in the tests is fine, we can keep this code for sure, but I do think we should eventually write code that can take UserPreferences and output an actual config object for us to start roundtripping, rather than relying on manually testing the editor to make sure the parsing code works.
| if v, ok := value.(map[string]any); ok { | ||
| // vscode's inlay hints settings are nested objects with "enabled" and other properties | ||
| switch name { | ||
| case "parameterNames": |
There was a problem hiding this comment.
We are case sensitive in some places, but not in others?
There was a problem hiding this comment.
case sensitive is for vscode specifically (they have specific cases that aren't in the typescript-language-server api, so i don't think we should support them elsewhere)
| } | ||
| 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.
| userPreferences := s.session.UserPreferences().CopyOrDefault() | ||
| if parsed := userPreferences.Parse(params.Settings); parsed != nil { |
There was a problem hiding this comment.
I find this pattern odd; shouldn't Parse be a top level func that takes in the params and then returns user prefs, rather than having to have an existing one, copy it, then parse over top of it?
There was a problem hiding this comment.
I did it this way because for both initalization/config requests and didChangeConfig notifications, we don't expect most of the preferences to be specified in the vast majority of cases. This means that most of the time, we would be merging a couple of parsed preferences into either an existing or a default preference, and doing "parsing over" makes it a lot easier to handle defaults/only change specified preferences in didChangeConfig
| 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...
typescript.preferences, or a flat json like typescript-language-servertodo:
javascriptformat