fix: add support for determing workspace folder with root uri/path on initialize #1210
Conversation
shruti0085
requested review from
breedloj and
a team
and removed request for
a team
| return [] | ||
| } | ||
| const getFolderName = (uri: string) => path.basename(URI.parse(uri).path) || 'workspace' | ||
| if (params.workspaceFolders) { |
There was a problem hiding this comment.
Also check if the array is empty, so that we avoid returning an empty list too early
| import * as path from 'path' | ||
| import { URI } from 'vscode-uri' | ||
|
|
||
| export function getWorkspaceFolders(params?: InitializeParams): WorkspaceFolder[] { |
There was a problem hiding this comment.
Should this method be wrapped in a try catch, returning an empty array if there is an error? In cases like VS, the inputs here would be out of the extension and user's control but we may not want the server to crash (or fail to initialize).
| import * as path from 'path' | ||
| import { URI } from 'vscode-uri' | ||
|
|
||
| export function getWorkspaceFolders(logging: Logging, params?: InitializeParams): WorkspaceFolder[] { |
There was a problem hiding this comment.
why is this in localProjectContext/ instead of shared/ ? seems generally applicable , not specific to project context.
|
|
||
| export function getWorkspaceFolders(logging: Logging, params?: InitializeParams): WorkspaceFolder[] { | ||
| try { | ||
| if (!params) { |
There was a problem hiding this comment.
nit: we can move the early return statements outside of the try catch block
if(!params){
return []
}
if (params.workspaceFolders && params.workspaceFolders.length > 0) {
return params.workspaceFolders
}
try{
...
| if (!params) { | ||
| return [] | ||
| } | ||
| const getFolderName = (uri: string) => path.basename(URI.parse(uri).fsPath) || 'workspace' |
There was a problem hiding this comment.
why do we default to workspace here? Couldn't it lead to some unexpected side effects?
There was a problem hiding this comment.
just as a default incase a folder name cannot be extracted. I can default it to the full path if that makes more sense
There was a problem hiding this comment.
Updated to return the uri instead as default
| return [{ name: folderName, uri: params.rootUri }] | ||
| } | ||
| if (params.rootPath) { | ||
| const pathUri = URI.parse(params.rootPath).toString() |
There was a problem hiding this comment.
it is slightly redundant that for this code path we do uri.parse twice. We do uri.parse() -> toString -> uri.parse()
There was a problem hiding this comment.
Reduced the duplication and refactored it a bit
| } | ||
| return [] | ||
| } catch (error) { | ||
| logging.error(`Error occurred when determine workspace folders: ${error}`) |
…ChangeEvent (#509) Currently in a lot of downstream servers, the `workspace.getWorkspaceFolder` method is used for path resolution. This only relied on the `initializeParams.workspaceFolder` value which did not take into account workspace folder changes(did not listen to didChangeWorkspaceFolders) that could have happened during the session. In addition with VS client as mentioned in aws/language-servers#1210, the workspaceFolder value is not set(more details in that PR) however it does specify that information via `rootUri` and rootPath. The utility class from that PR has now been moved centrally to this repo. A new `getAllWorkspaceFolders()` call has been added as a common utility that servers downstream can use as single source of truth to get latest information of workspaceFolders available to the lsp server. A follow-up PR to language-servers repo will utilize this change.
Description
LSP lifecycle for VS extension is controlled by Visual Studio. It currently does not set the
workspaceFolderas part of theInitialize Paramsof the LSP initialize request, however it does send this information viarootUriandrootPathproperties. The workspaceFolders is utilized for the local project context indexing and so in order to support that, this change extends logic to determine the workspaceFolder to use the root properties when workspaceFolders is not set.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.