Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol extension to support workspace folders. #298

Closed
dbaeumer opened this issue Sep 12, 2017 · 29 comments
Closed

Protocol extension to support workspace folders. #298

dbaeumer opened this issue Sep 12, 2017 · 29 comments
Labels
feature-request Request for new features or functionality

Comments

@dbaeumer
Copy link
Member

Many tools support more than one root folder per workspace. Examples for this are VS Code's multi-root support, Atom's project folder support or Sublime's project support. If a client workspace consists of multiple roots then a server typically needs to know about this. The protocol up to know assumes one root folder which is announce to the server by the rootUri property of the InitializeParams. A protocol extension is proposed to add first class workspace folder support. A markdown documentation is here and a reference implementation for the client and server

@dbaeumer dbaeumer added the feature-request Request for new features or functionality label Sep 12, 2017
@david-driscoll
Copy link

Seems like a sensible addition.

Will this be targeting 4.0?

@dbaeumer
Copy link
Member Author

@david-driscoll since it is no breaking and the capability is flagged by the client and by default false I think we can add this to a 3.x version.

Implementations are available in the vscode-language* npm modules. But you have to opt into them.

@dbaeumer
Copy link
Member Author

@object88
Copy link
Contributor

If I am reading the documentation correctly, then if a client supports multiple workspaces, then it will announce that support during the init. But there still will only be one rootUri provided in that init request.

In my implementation, I am immediately responding to the initialization request, and do some background processing to start churning the code in the initial rootUri. I queue up any further requests coming from the client until after I have finished my background processing, then I start responding.

I am imagining that if the client says that it supports multiple workspaces, I would like to know what those workspaces are immediately, so that I can add that to my background processing. I'd like to know if there's any (theoretical) issues if I send a workspace/workspaceFolders request from the server to the client if I have not yet started processing the client's requests?

@svenefftinge
Copy link
Contributor

I understood, that the list of workspaceFolders is sent as part of the InitializeParams : https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.workspaceFolders.proposed.ts#L12

The workspace/workspaceFolders request is merely there to react to changes (i.e. workspace/didChangeWorkspaceFolders).

@dbaeumer
Copy link
Member Author

@svenefftinge is correct. The list of folders comes in the initialize request as well.

@object88
Copy link
Contributor

That's awesome, and will keep my startup code more simple. Thanks @svenefftinge and @dbaeumer .

@mickaelistria
Copy link

I don't see a serverCapability associated with the support for multiple folders in the proposal. Then, if the client supports multiple folders and initialize with this request capability, how can the client know whether the server can support multiple folders to decide whether to reuse it across multiple projects or to spawn a new instance for each project?
Is this server capability just missing or is there a better way to check from client that server can support multiple folders?

@mickaelistria
Copy link

About previous comment, in the proposal, it introduces a capability to register. While this can be useful in many case, registering capabilities is an overhead in the flow, and we could expect the support for modifiable multiple root folders to be declared as a ServerCapability.

@mickaelistria
Copy link

See microsoft/vscode-languageserver-node#272 for a proposal to support it in ServerCapabilities as well.

@dbaeumer
Copy link
Member Author

@mickaelistria having this server capability makes sense. Can you please sign the CLA otherwise I can't merge it.

@mickaelistria
Copy link

@dbauemer I signed CLA earlier today and the bot says "all CLA are met". Anything else I'm missing?

@dbaeumer
Copy link
Member Author

Nothing else. It still has the CLA required label but I will remove it.

@mickaelistria
Copy link

mickaelistria commented Nov 10, 2017 via email

@dbaeumer
Copy link
Member Author

Yes that is correct.

@dbaeumer
Copy link
Member Author

The structure looks a little different to support future growth of the properties:

export interface WorkspaceFoldersServerCapabilities {
	/**
	 * The workspace server capabilities
	 */
	workspace?: {

		workspaceFolders?: {

			/**
			 * The Server has support for workspace folders
			 */
			supported?: boolean;

			/**
			 * Whether the server wants to receive workspace folder
			 * change notifications.
			 *
			 * If a strings is provided the string is treated as a ID
			 * under which the notification is registed on the client
			 * side. The ID can be used to unregister for these events
			 * using the `client/unregisterCapability` request.
			 */
			changeNotifications?: string | boolean;
		}
	}
}

@keegancsmith
Copy link

A suggestion: can we call it WorkspaceRoots instead of WorkspaceFolders. Roots is consistent with the current initialise parameters in LSP. Apologies for potentially bike shedding :)

@dbaeumer
Copy link
Member Author

@keegancsmith The initialize paramater uses workspaceFolders as well. See https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.workspaceFolders.proposed.md.

This has been like this since the first commit. Anything I miss.

@keegancsmith
Copy link

Sorry for the confusion, I was referring to the existing InitializeParams https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#initialize outside of this proposal. It's not a major point, but in the current spec we call it a rootUri. The proposed spec we refer to them as workspaceFolders. I was thinking it would be more consistent to use the same terminology and just call them workspaceRoots instead. However, this is not important; I am late to comment that; and I realise that using the word folder is consistent with what vscode calls the concept.

@rcjsuen
Copy link
Contributor

rcjsuen commented Nov 15, 2017

I don't think that being consistent with Visual Studio Code should necessarily have a bearing on what is ultimately picked as a name here. The point of the language server protocol is to be client agnostic. Also note that while VS Code may call them folders in the user interface, the feature itself is advertised as "multi root workspaces".

@dbaeumer
Copy link
Member Author

workspaceRoots would be ok as well. However I opt to leave it like it is.

@mickaelistria
Copy link

For the capability, do we expect it to be ServerCapabilities.workspace.workspaceFolders.supported like in the current state, or just ServerCapabilities.workspaceFolders.supported. I tend to prefer the later one but both would work for me so I think you @dbaeumer should be the one to decide here.

@dbaeumer
Copy link
Member Author

It is ServerCapabilities.workspace.workspaceFolders.supported. After discussing with @aeschli we decided to make this more structural to better support future evolution.

@krzyzanowskim
Copy link
Contributor

What is a relation between rootUri and workspaceFolders. If a client provide rootUri but not workspaceFolders, does it mean that single workspaceFolder is configured?
And in the other way, if rootUri is null but workspaceFolders is provided. Is the first path from the workspaceFolders equivalent of rootUri ?

@DanTup
Copy link
Contributor

DanTup commented Jan 9, 2019

@dbaeumer Is this complete? It's in the spec like it is, but this issue is still Open.

Btw - there may be some bad text in the spec for this:

The notification is sent by default if both ServerCapabilities/workspace/workspaceFolders and ClientCapabilities/workspace/workspaceFolders are true; or if the server has registered to receive this notification it first.

The "it first" at the end doesn't make sense to me (maybe it should be "at first" or the word "it" removed, though I'm still not sure what it means.

@dbaeumer
Copy link
Member Author

dbaeumer commented Jan 9, 2019

@krzyzanowskim if more than one workpace folder is provided there is no rootUri. To keep the protocol compatible the client decides in a multi folder setup which folder is the root folders (got for example be a user decision). The client would provide both properties rootUri and workspaceFolders. So a server that is only root aware (one folder) only takes that property.

@DanTup thanks for pointing out the incomplete sentence. Fixed it.

I will close the issue since this is indeed implemented.

@dbaeumer dbaeumer closed this as completed Jan 9, 2019
@krzyzanowskim
Copy link
Contributor

@dbaeumer one more question. Who's responsibility is to close an open document for a removed folder. I guess the client should notify the server, and close the files from the folder that is about to be removed. Thoughts?

@DanTup
Copy link
Contributor

DanTup commented Jan 14, 2019

Who's responsibility is to close an open document for a removed folder.

As I understand it, it's fine to have open files outside of the the open folders so I don't think these two things are related. If an editor chooses to close files when a workspace folder is removed, it should send events for them. If it does not, the server shouldn't make any assumptions about them being closed.

@dbaeumer
Copy link
Member Author

@krzyzanowskim @DanTup is correct. It depends on what the editor decides to do.

@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

9 participants