Vscode: 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 https://github.com/Microsoft/vscode/issues/28526#issuecomment-308177541

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?

[sandy081]

With rootPath to undefined in multi-root set up causes GIT extension to be disabled. This blocks to self host on multi root.

[jrieken]

I read that we should go back to making it the first workspace folder

[jrieken]

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)

Maybe we need to rethink this. Today, after making rootPath behave like that, we have seen issues with https://github.com/KnisterPeter/vscode-github which activates via workspaceContains:.git and fails (throws errors) because rootPath is not defined. With what we have to day, the extension is doing everything correct and we should not break it like this. So, it might be better to have a somewhat stable rootPath, e.g. the first root folder by some stable sort order, and restart only when that goes away. Deprecation and new API obviously still remains

[jrieken]

fyi @Microsoft/vscode The alternative proposal is to make the first workspace the root folder. In our current implementation that would be the master folder but in an implementation with a list of equal folders (ala vscode-project.json) it would be the first (index=0) folder. Only when that folder goes away (is getting closed/removed) we would need to restart the extension (which today in only possible by reloading the window).

[Tyriar]

The alternative proposal is to make the first workspace the root folder.

@jrieken the whole "master folder" concept thing is a concept not many people like and is pending @bpasero and @sandy081 I think looking into how we are going to persist the workspaces in the long run.

The concern with making it the first workspace is that the extension will work for one folder and potentially one folder only, and therefore not behave as expected for the non-"master folders". This is what I've been envisioning for the rollout:

1. Introduce new APIs, deprecate rootPath (still use the first folder)
2. Notify extension authors about the API change if they use rootPath in their extension
3. Assist prominent extensions with adapting to the new API
4. Indicate that extensions may not operate properly (eg. if they poke rootPath, show a warning message box or a indicator in the extension viewlet)
5. (Optional) Switch rootPath to be undefined once adoption is satisfactory (maybe after 2-3 months?)

If we do not do step 5 in the long term then there will always be extensions that are buggy and misbehave in a multi-root scenario, rather than providing their empty workspace behavior that should be more reliable/stable (even if the functionality is disabled completely). Perhaps doing step 5 may also encourage authors more to go and fix up their extension to support multi-root?

[jrieken]

@jrieken the whole "master folder" concept thing is a concept not many people like and is pending @bpasero and @sandy081 I think looking into how we are going to persist the workspaces in the long run.

Don't get me wrong I fully agree that master isn't what we want. That's why I didn't say master but first 🙊.

Introduce new APIs, deprecate rootPath (still use the first folder)

Yep, that is still the plan but we don't want to break existing extensions using it by making it defunct. So, using the first folder is the don't-break-strategy but not the future.

[jrieken]

re https://twitter.com/k_cieslak/status/877854547238170624 @Krzysztof-Cieslak lets continue the discussion here, with more than 160 character available :-)

[Krzysztof-Cieslak]

As I've mentioned to @Tyriar in other response thread to my tweet I have 2 main issues:

• rootPath is used quite a lot in my plugins (I've counted 20 something usages in 4 plugins I maintain) - some of them can probably be replace by some combinations of workspace.findFiles and new Workspace APIs, but still it will require decent amount of work (just because old API is used in many different functionalities, breaks some assumptions that were made, it's change from sync to promise based API, etc). It's definitely doable, it's not probably something super hard to do conceptually but just plain amount of changes will be problematic (given those are OSS projects I do in spare time - and I guess same is true for any other big plugin that is not luck enough to be supported by any corporation ;) )

• At least in case of F# (and possibly Elm - I'd need to check it out) the language server (which is not implementing LSP) is designed for "single workspace" approach. From top of my head there are 2 possible approaches - either I add multiple workspace to language server which is lot of work on server side (and also is problematic since VSCode plugin is not only user of this language server) , or I spawn separate server for each workspace which requires me to do decent amount of changes in plugin side - either way those would be not super trivial changes and will require decent amount of work.

  I guess what I've meant in tweet is not "breaking" as defined by "my extensions stops working day after you release it" (unless you set rootPath to undefined - this would be for sure breaking change), but rather I've meant that It will be really lot of work to make plugins support multiple workspaces.

From my point of view the 5 points plan that @Tyriar suggested above seems fine. We need those few months transitions period to port extensions to new API and then test them enough.

[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