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

IEnvironmentService#userHome is deprecated #94506

Closed
bpasero opened this issue Apr 6, 2020 · 17 comments
Closed

IEnvironmentService#userHome is deprecated #94506

bpasero opened this issue Apr 6, 2020 · 17 comments
Assignees
Labels
debt Code quality issues remote Remote system operations issues web Issues related to running VSCode in the web
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Apr 6, 2020

I recently cleaned up the environment service for web and one outcome is that userHome is actually not known upfront until resolved later from the remote in web. For that case we do have a IRemotePathService#userHome that checks the remote and falls back to the local user home if possible.

I would like people to adopt the remote path service if possible. One catch is that the method is async since it needs to resolve the remote environment if there is one.

Outcome of this should be that no-one depends on IEnvironmentService#userHome anymore.

@bpasero bpasero added debt Code quality issues remote Remote system operations issues web Issues related to running VSCode in the web labels Apr 6, 2020
@sandy081
Copy link
Member

sandy081 commented Apr 6, 2020

@bpasero In ExtensionTipsService, it is being used to check local installables. I already have a check for web and do not do these checks. I only want to look for installables on local machine and I do not want to check installables on remote environment. Any suggestions how to go forward?

@bpasero
Copy link
Member Author

bpasero commented Apr 6, 2020

Hm, why would we not show an extension tip checking for installed tools on the remote?

@sandy081
Copy link
Member

sandy081 commented Apr 6, 2020

We cannot treat all remote environments same. For eg., Dev containers what does it mean looking for installables in it? Does it makes sense?

I also think it is not intended to look for different kind of installables in the remote machine and prompt them in the remote window which does not need them at all. I see remote window is scoped to a workspace.

@bpasero
Copy link
Member Author

bpasero commented Apr 6, 2020

If you feel that web/remote should not have these tips, then I suggest to split the service into 2: web and native and only do the one in the one case.

@isidorn
Copy link
Contributor

isidorn commented Apr 6, 2020

@bpasero labelService depends on this being sync, if we change this method to be async than all callers of labelSerivce.getUriLabel will have to become async and that pretty much means the whole world. Is there an alternative approach here?

@Tyriar Tyriar removed their assignment Apr 6, 2020
@bpasero
Copy link
Member Author

bpasero commented Apr 6, 2020

@isidorn I guess, the alternate approach is to have sync access where userHome essentially becomes URI | undefined, that is it will be undefined until resolved (probably indicated with some event)?

Still, I would not want to have this on the IEnvironmentService, rather the remote path service as today.

@isidorn
Copy link
Contributor

isidorn commented Apr 6, 2020

@bpasero that makes sense. If you introduce that URI | undefined call and the event in the remotePathService I will happily start using it.

bpasero added a commit that referenced this issue Apr 6, 2020
@bpasero
Copy link
Member Author

bpasero commented Apr 6, 2020

@isidorn I added IRemotePathService#userHomeSync

@isidorn
Copy link
Contributor

isidorn commented Apr 6, 2020

@bpasero thanks.
@weinand fyi since I did changes in loadedScriptsView

isidorn added a commit that referenced this issue Apr 6, 2020
sandy081 added a commit that referenced this issue Apr 6, 2020
- do not recommed exe and dynamic workspace recommendations in web
@sandy081 sandy081 assigned isidorn and unassigned sandy081 and isidorn Apr 6, 2020
roblourens added a commit that referenced this issue Apr 8, 2020
Would be better to make the querybuilder async but that has a huge downstream impact
@roblourens
Copy link
Member

Did this in the QueryBuilder - tried to make it async to do this properly but it spiraled out of control.

@alexr00
Copy link
Member

alexr00 commented Apr 9, 2020

@bpasero what is the preferred way to get the local userHome even when there is a remote userHome now? For the simple file picker I need both.

@bpasero
Copy link
Member Author

bpasero commented Apr 10, 2020

@alexr00 you can always depend on INativeEnvironmentService to get the userHome which will be of type URI and never undefined. My plan is to remove userHome from the normal IEnvironmentService and only have it in the native part. That is the only place where it is and can be defined.

@bpasero bpasero assigned bpasero and unassigned roblourens and Tyriar Apr 10, 2020
@bpasero bpasero added this to the April 2020 milestone Apr 10, 2020
@bpasero
Copy link
Member Author

bpasero commented Apr 10, 2020

Since we are getting close, assigning myself for final cleanup and moving to April.

@Tyriar
Copy link
Member

Tyriar commented Apr 10, 2020

FYI my refactor broke the SSH extension as local terminals need to be launched before the remote connection is available. Moving to userHomeSync.

Tyriar added a commit that referenced this issue Apr 10, 2020
roblourens added a commit that referenced this issue Apr 10, 2020
alexr00 added a commit that referenced this issue Apr 14, 2020
Changes for simplefiledialog.ts
Part of #94506
@alexr00
Copy link
Member

alexr00 commented Apr 14, 2020

@bpasero I had to move some things around to not break layering and use INativeEnvironmentService in simplefiledialog.ts. Pull request #95198

alexr00 added a commit that referenced this issue Apr 14, 2020
Part of #94506

Co-authored-by: Alex Ross <alros@microsoft.com>
@alexr00
Copy link
Member

alexr00 commented Apr 14, 2020

Simple file dialog is done.

@alexr00 alexr00 removed their assignment Apr 14, 2020
@bpasero
Copy link
Member Author

bpasero commented Apr 15, 2020

I decided to go with the following solution:

  • IRemotePathService is renamed to IPathService and should be used in all cases where you want the correct path behaviour for both remote and local environments
  • there is userHome which is Promise<URI> that will always be defined and either be the remote user home or local (even for web serverless there will be a good fallback: vscode-remote:<remote authority>/)
  • there is resolvedUserHome which is of type URI | undefined that is being resolved on startup but maybe undefined until resolved

There is a slight semantic change from the old behaviour: if you used to use IEnvironmentService#userHome and now switched to IPathService#resolvedUserHome you will get an undefined user home on VSCode desktop, even if a local userHome is present for as long as the remote environment is not resolved. If you must fallback to the local userHome, you need to use the native variants of the environment service yourself.

//cc @isidorn @alexr00 @Tyriar @roblourens for potential customers

PS: noticed quite some usages of userHome.fsPath, I think that is not 100% correct because fsPath might not work well with remote URIs. I think fsPath should only be used for file URIs.

@bpasero bpasero closed this as completed Apr 15, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues remote Remote system operations issues web Issues related to running VSCode in the web
Projects
None yet
Development

No branches or pull requests

7 participants