API and extension host work for multi root

[jrieken]

This is an umbrella item that track changes that are required for making the extension host and API multi-root aware.

[jrieken]

Open questions and Todos

1. We have the workspaceContains-activation event and I assume we (re-)evaluate whenever a new folder comes in? @alexandrudima for feedback/input
2. The value of vscode.rootPath is computed at startup time but now it might change (when we close the folder that we have chosen to be the rootPath). What do we do with that? fyi @bpasero
3. We have extension storage path which we today derive from the workspace identifier and need something comparable in the future

[bpasero]

@jrieken for now since we have the model of a main folder that has an associated setting, rootPath would always point to this folder and not change unless another folder is opened (that would restart the extension host). We should then deprecate rootPath in favour of new API that has a list of folders and an associated event.

An alternative solution would be to simply set rootPath to null in case multiple folders are opened so that extensions are in the same situation as when the user had no folder opened.

[jrieken]

Yeah, for now I have no problems, but as soon as remove and add all folder I do. Also setting rootPath to null or undefined is tricky because it will come after the fact, e.g. I start with one folder and then add other later

[bpasero]

@jrieken good point, I forgot about the fact that we restart the extension host typically in that case.

Maybe then it is better for now to just always have rootPath be the folder path of the main folder the user opened and the new API to be dynamically changing based on the configuration?

[jrieken]

just always have rootPath be the folder path of the main folder the user opened

Unsure, it would bake the notion of a main folder into the API

[bpasero]

@jrieken that is true, but I see no way of setting rootPath dynamically if we are not able to reload the extension host in the background. Otherwise this would mean each time a root folder changes we have to reload the entire window.

[alexdima]

Regarding workspaceContains, this is now evaluated only at folder opening time. In other words, if an extension is looking for a file called hello.txt, and a folder is opened without that file, and later a file called hello.txt is created, the extension does not get activated.

I agree, the natural mapping of workspaceContains is to evaluate it on each of the opened folders, at folder opening time.

[bpasero]

After talking to @jrieken today there are some ideas what to do with our current rootPath extension API once we support multi root:

Set rootPath to master
In this case, workspace.rootPath always has the value of the folder that was opened that carries the additionalFolders setting. This would require a minimal amount of work because multi-root always requires a folder to be opened to begin with. The disadvantage of this approach is that it "leaks" the concept of the master folder to our extensions.

Set rootPath to null in multi-root
Here we simply set workspace.rootPath to null in case more than one folder is opened. Extensions today already have to deal with this when the user opens an empty workspace without folder. This has the advantage of making clear that multi-root is a new concept that requires the extension to adopt new API. There is some work involved though to make this experience smooth: We cannot change rootPath dynamically, so any change in the multi-folder configuration will trigger a reload of the window (for transitions from 1 to many or many to 1). We need to invest into a way of being able to restart the extension host in the background but this is out of scope for MVP.

Remove rootPath for extensions > 1.14
In this model we forward-break our API by removing workspace.rootPath beginning with our next release. For each extension we can detect if it supports 1.14 and then decide to not populate workspace.rootPath at all. For extensions that are still supporting older versions we would provide workspace.rootPath as before with one of the 2 solutions outlined above. The advantage here is that new extensions (those that support 1.14) would not even see workspace.rootPath as API but only the new workspace API we provide.

@Microsoft/vscode feedback welcome

[bpasero]

I am personally in favour of setting rootPath to null when opening more than one folder. This was already outlined as proposal in @Tyriar and mine document about multi root support.

[chrmarti]

I'd set rootPath for old extensions to keep them maximally functional and remove rootPath for newer extensions to force a clear cut so extensions do not end up depending on both old and new APIs.

[jrieken]

So, the very cut of multi-root API is small and just the list of folders and an event when that list changes. Also, I have changed workspace.rootPath to be undefined when there is more than 1 workspace.

export namespace workspace {

	export const onDidChangeWorkspaceFolders: Event<Uri[] | undefined>;

	export let workspaceFolders: Uri[] | undefined;
}

[bpasero]

After talking in todays multi-root sync we decided to go with the solution of Set rootPath to undefined in multi-root described in #28526 (comment)

While the forward breaking is a compelling idea, it needs more infrastructure that we do not have currently. Once we have this, we would use it to remove any deprecated API actually that we have.

For now we will deprecate rootPath in the vscode.d.ts and update the documentation where we still reference it.

Kai pointed out that we must reserve at least one sprint for extensions to adopt the new API. We should not make multi root broadly enabled without having a significant number of our top extensions having it adopted. Enabling multi-root only in insiders for now gives extension authors a chance to adopt the new APIs.

[jrieken]

Set rootPath to null in multi-root

Please stop saying null when it is undefined 🙃

[egamma]

@bpasero

• what is the user experience for users when they go from 1 to 2 root folders and they have extensions installed that are using the now deprecated API? If I remember properly you had some mocks for this scenario?

• what do we do with extensions that were not updated after the one sprint adoption period? My concern is that there are extensions that are popular and in use, but that are no longer actively maintained.

[dbaeumer]

I personally find a one sprint adoption period very short especially given the fact that this falls into the summer month and extension author might be on summer vacation.

[sandy081]

When I opened my multi root workspace today with latest insiders, I see following error

And this is coming from an extension.. Probably we should handle this nicely, convey some how this is multi root and extension does not support it?

[Krzysztof-Cieslak]

Also as a side note - even if you remove rootPath eventually it won't matter as much since least effort workaround will be using workspaceFolders[0] as substitution for the rootPath.... my F# Atom plugin still does that even though they've introduced multiple folders support years ago ;)

[jrieken]

That is correct, we cannot enforce that an extension uses all workspace information that is available and if an extension only works on the first folder that's the way it works. I think that is just the nature of new, major features coming late to the party; like multi-root. The reverse is that it doesn't make much sense to set rootPath to undefined. We liked that idea because already today it can be undefined and extensions should be fit, in the sense of not explode, when this happens. However, the consequence is that those extensions will work in single-file-mode when there are multiple folders and I don't think that is better than having them work in single-folder-mode.

So, I think depicting the first folder as rootPath and restarting the extension host when that folder changes in the best compromise. The way how smart extensions work shouldn't change by that. And btw we did push those fixes already yesterday.

[Krzysztof-Cieslak]

We liked that idea because already today it can be undefined and extensions should be fit, in the sense of not explode, when this happens. However, the consequence is that those extensions will work in single-file-mode when there are multiple folders and I don't think that is better than having them work in single-folder-mode.

Sure we do check rootPath for undefined and we do have single-file-mode.... But it would give really bad UX - it's rather clear when VSCode is opened in single file mode, so it's obvious that some extensions may not work. With multiple workspaces it's not as obvious, whole editor behaves normally, just for some random reason (from user perspective) extension doesn't work.

So, I think depicting the first folder as rootPath and restarting the extension host when that folder changes in the best compromise.

This sounds good, indeed.

[bpasero]

@egamma no mockups for this scenario yet because "Multi Root Light" was about keeping rootPath around, even when going into multi root. It looks like we will have to work on the UX for this though once we set rootPath to undefined.

Giving extensions a longer period of time to adopt this change makes sense to me. I also agree that one sprint will not be enough, especially during summer.

@jrieken regarding the workspaceContains activation issue with multi root: now we already adopted multi root for this event (i.e. if any of the roots contain a file of choice, the extension gets activated). we still however set rootPath to the first workspace. Is this not another vector of breaking extensions in case not the first workspace contains the file? I have the feeling that we also have to deprecate workspaceContains and let it work as before (i.e. only look at the workspace that is the same as rootPath) and have a new activation event that is multi-root aware.

[jrieken]

After discussions with @weinand we decided to represent a workspace folder as dedicated object, for now like so. With that life is already a little easier and it also allows for future growth of the API.

export interface WorkspaceFolder {
	readonly uri: Uri;
	readonly name: string;
	readonly index: number;
}

Also, we added a way to find a workspace folder given a certain uri. /cc @mjbvz - I believe you have a ghetto-style implementation of that.

export function getContainingWorkspaceFolder(uri: Uri): WorkspaceFolder | undefined;

[bpasero]

Sounds good to me. @jrieken is the index property identical to the position in the workspaceFolders array? If so, I wonder why it needs to be so prominent.

[jrieken]

Yeah, the index is meant to represent the order, as defined in the code-workspace-file, and merely for convenience. @weinand has a good use-case which is about sorting a derived tree (of loaded scripts) like we sort the workspace folder list. This saves you a workspaceFolders.indexOf(someWorkspaceFolder) which is esp helpful when object instances have changed after processing some event. Tho, open for more feedback on this

[mjbvz]

@jrieken I like the idea of getContainingWorkspaceFolder but TS may be a bit weird when it comes this sort of API for nested folders. For a folder structure:

base/
    a.js
    sub/
        b.js

Where the user opens folder root and sub, for the file b.js TS needs the most broad enclosing project, which would be root in this case instead of sub. I imagine this is the opposite of what getContainingWorkspaceFolder would normally do

[roblourens]

Sorry, how is getContainingWorkspaceFolder different from IWorkspaceContextService.getRoot?

[jrieken]

Sorry, how is getContainingWorkspaceFolder different from IWorkspaceContextService.getRoot?

It's no different, just uses the API types and a more explicit name.

Where the user opens folder root and sub, for the file b.js TS needs the most broad enclosing project, which would be root in this case instead of sub. I imagine this is the opposite of what getContainingWorkspaceFolder would normally do

That is interesting. You might be able to call this n times... So b.js -> sub/ and then sub/ -> base/. That could or should work.

[jrieken]

... So b.js -> sub/ and then sub/ -> base/. That could or should work.

It doesn't work today but we could make it work. Alternative could to let WorkspaceFolder have a parent pointer. Tho not sure I like that, it's a corner case and might make things look more complex than necessary.

[jrieken]

Pushed more tweaks and moved the API out of proposed. fyi @mjbvz The getWorkspaceFolder-function now returns a parent workspace if called with one: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/test/electron-browser/api/extHostWorkspace.test.ts#L105