support multi-root workspaces

[chrisdias]

No description provided.

[chrisdias]

testing: microsoft/vscode#30722
reference: microsoft/vscode#28526

in commands\build-image.ts:

function createItem(uri: vscode.Uri): Item {
    let filePath = hasWorkspaceFolder() ? path.join(".", uri.fsPath.substr(vscode.workspace.rootPath.length)) : uri.fsPath;

    return <Item>{
        description: null,
        file: filePath,
        label: filePath,
        path: path.dirname(filePath)
    };
}

We use the rootPath length to trim the rootPath off of the file name to provide a clean list relative to the workspace. This breaks with multi-root workspaces:

[weinand]

You can use this to trim a path:

const trimed = vscode.workspace.asRelativePath(path);

asRelativePath has been updated to handle the multi-root case.

/cc @bpasero

[bpasero]

@chrisdias besides the obvious usage of rootPath, do you expect any settings to be folder scoped?

[chrisdias]

No, I think everything can be "global". We use rootPath to find Dockerfile and docker-compose.yml files and send the correct paths along to the docker CLI.

[egamma]

@bpasero @chrisdias the docker extension is using a language server for docker. The experience from @dbaeumer and me is that we could make all language server settings resource scoped. I suggest to consider the same for the docker extension.

[chrisdias]

ah yes, completely forgot about the new language server. that would make sense for the language server to be scoped to a folder.

//cc @rcjsuen

[rcjsuen]

@egamma Hi Erich, thank you for raising this issue to my attention.

It's currently not clear to me how this should be be implemented. I can see that workspace folder support is currently a proposed extension to the language server protocol but I haven't been able to find any sample code that shows how to distinguish whether a setting is for workspace A or workspace B. Are there any articles somewhere that I should be reading up on?

[bpasero]

@rcjsuen see https://github.com/Microsoft/vscode/wiki/Extension-Authoring:-Adopting-Multi-Root-Workspace-APIs

[egamma]

@rcjsuen the CSS server is a good example of how to use the proposed protocol https://github.com/Microsoft/vscode/blob/master/extensions/css/server/src/cssServerMain.ts

In your current implementation the settings are pushed to you in onDidChangeConfigurationSettings. In the multi folder implementation you pull the settings for a particular resource from the server using the GetConfigurationRequest call.

Also when you look into this we would appreciate feedback on the migration documentation and how we can improve it.

[bpasero]

@chrisdias looked at some commands we expose (via e46610d). These commands seem to require a opened workspace, is that true? If so, they should get the necessary code early on to return when no workspace is opened, e.g. by telling the user. The commands are:

• vscode-docker.image.build (Build a Docker image from a Dockerfile)
• vscode-docker.compose.up (Starts a composition of containers)
• vscode-docker.compose.down (Stops a composition of containers)
• vscode-docker.configure (Add Dockerfile, docker-compose.yml files)

[rcjsuen]

I tried using 1.17.0-insider (microsoft/vscode@7ac1f1e05) a little for looking at the new API today but it crashed both times within minutes with no error messages or anything. Where should I look to try and find more information about these crashes?

[rcjsuen]

I've gotten the language server to work locally against an Insiders build but I'm seeing an error from a fake LSP client. Not sure if it's something on my end or if vscode-languageserver-node is not tolerant enough when trying to read the initialization request from the client.

{
  jsonrpc: '2.0',
  id: 1,
  error: {
    code: -32603,
    message: 'Request initialize failed with message: Cannot read property \'workspaceFolders\' of undefined'
  }
}

[bpasero]

@dbaeumer maybe has a clue

[rcjsuen]

Thanks Benny. I figured out why and opened microsoft/vscode-languageserver-node#257.

[rcjsuen]

Tagged and released v0.0.8 of dockerfile-language-server-nodejs to npm. Thank you for the pointers, @egamma and @bpasero.

Here are two things that I noticed while trying to hammer this out today:

1. Trying to get the server to work with both classic clients and the Insider build was not easy. The example assumes that you are only trying to do things the new way. This doesn't make sense because the reason we write language servers is so that multiple clients can connect to them and not because we only want to connect to VS Code.
2. The aforementioned link mentions this but you need to remember to set "scope": "resource" in your package.json or your settings won't function in a hierarchical manner.

I feel like there were several other things like unresolved promise errors that "made no sense" (at the time) but I think a lot of it came down to my first point. The example helped me understand how to write the new code but it wasn't clear to me where my if/else statements should start and end after checking the initialization parameters to determine whether workspace/configuration was supported or not.

[rcjsuen]

I was trying to finalize the changes required for the client when I realized that I missed something during my pass of the server code (rcjsuen/dockerfile-language-server#182). I feel like a section should be defined when the server sends a workspace/configuration request instead of leaving hard coded values in some middleware bridge code on the client.

However, hard coded values is exactly what's used in the sample client's code. @dbaeumer Why does the server choose to send in empty strings and why does the client explicitly forbid this by ignoring defined section names and returning null?

[egamma]

@rcjsuen great progress!

@dbaeumer see also this comment above #101 (comment) regarding the sample.

@rcjsuen if you have suggestions for how to improve the wiki page please let us know (PRs for the wiki page are in particular welcome). We plan to reach out to other extension authors during this iteration.

[dbaeumer]

@rcjsuen since a server should in principal work with any kind of clients defining settings sections and directly reading them from the client's settings store works only for one type of client.

With the introduction of the getConfiguration request's in the LSP the server is now responsible to define its setting structure. You are free to use any structure you want including the empty section as we do in the example. Since the server sends the request to the client it is clear which settings are ask for. In your example it is the docker server asking for its settings. No need to name them 'docker' in the request.

I choose the empty section name to make it clear that there is no need to have the settings defined by the server to be structural equivalent to the client's settings store. For example here is the translation for the ESLint server settings to how they are currently stored in VS Code's settings store.

					if (!params.items) {
						return null;
					}
					let result: (TextDocumentSettings | null)[] = [];
					for (let item of params.items) {
						if (item.section || !item.scopeUri) {
							result.push(null);
							continue;
						}
						let resource = client.protocol2CodeConverter.asUri(item.scopeUri);
						let config = Workspace.getConfiguration('eslint', resource);
						let settings: TextDocumentSettings = {
							validate: false,
							autoFix: false,
							autoFixOnSave: false,
							options: config.get('options', {}),
							run: config.get('run', 'onType'),
							nodePath: config.get('nodePath', undefined),
							workingDirectory: undefined,
							workspaceFolder: undefined,
							library: undefined
						}
						let document: TextDocument = syncedDocuments.get(item.scopeUri);
						if (!document) {
							result.push(settings);
							continue;
						}
						if (config.get('enabled', true)) {
							let validateItems = config.get<(ValidateItem | string)[]>('validate', ['javascript', 'javascriptreact']);
							for (let item of validateItems) {
								if (Is.string(item) && item === document.languageId) {
									settings.validate = true;
									if (item === 'javascript' || item === 'javascriptreact') {
										settings.autoFix = true;
									}
									break;
								}
								else if (ValidateItem.is(item) && item.language === document.languageId) {
									settings.validate = true;
									settings.autoFix = item.autoFix;
									break;
								}
							}
						}
						if (settings.validate) {
							settings.autoFixOnSave = settings.autoFix && config.get('autoFixOnSave', false);
						}
						let workspaceFolder = Workspace.getWorkspaceFolder(resource);
						if (workspaceFolder) {
							settings.workspaceFolder = {
								name: workspaceFolder.name,
								uri: client.code2ProtocolConverter.asUri(workspaceFolder.uri)
							};
						}
						let workingDirectories = config.get<(string | DirectoryItem)[]>('workingDirectories', undefined);
						if (Array.isArray(workingDirectories)) {
							let workingDirectory = undefined;
							let workspaceFolderPath = workspaceFolder && workspaceFolder.uri.scheme === 'file' ? workspaceFolder.uri.fsPath : undefined;
							for (let entry of workingDirectories) {
								let directory;
								let changeProcessCWD = false;
								if (Is.string(entry)) {
									directory = entry;
								}
								else if (DirectoryItem.is(entry)) {
									directory = entry.directory;
									changeProcessCWD = !!entry.changeProcessCWD;
								}
								if (directory) {
									if (path.isAbsolute(directory)) {
										directory = directory;
									}
									else if (workspaceFolderPath && directory) {
										directory = path.join(workspaceFolderPath, directory);
									}
									else {
										directory = undefined;
									}
									let filePath = document.uri.scheme === 'file' ? document.uri.fsPath : undefined;
									if (filePath && directory && filePath.startsWith(directory)) {
										if (workingDirectory) {
											if (workingDirectory.directory.length < directory.length) {
												workingDirectory.directory = directory;
												workingDirectory.changeProcessCWD = changeProcessCWD;
											}
										}
										else {
											workingDirectory = { directory, changeProcessCWD };
										}
									}
								}
							}
							settings.workingDirectory = workingDirectory;
						}
						result.push(settings);
					}
					return result;
				}
			}

[rcjsuen]

Thanks for the pointers, Dirk. I think I understand what you are trying to get at.

I currently have an issue where my language server isn't getting notified when a workspace folder's settings change. Maybe something has changed a little in the last two weeks so I'm going to grab the latest sample code and try to restart from scratch.

[rcjsuen]

I raised my problem on Slack and it's supposedly a bug. Please see microsoft/vscode#36625.

[rcjsuen]

I opened #156 to add support for the workspace/configuration request.

Please feel free to let me know if there's anything else that I still need to do for the language server.

[StephenWeatherford]

As far as I know, this work is completed, so closing. Thanks!