Skip to content

Handles script infos that dont exist on the disk and are opened with non-rooted disk path#19730

Merged
sheetalkamat merged 3 commits into
masterfrom
fileNotOnDisk
Nov 6, 2017
Merged

Handles script infos that dont exist on the disk and are opened with non-rooted disk path#19730
sheetalkamat merged 3 commits into
masterfrom
fileNotOnDisk

Conversation

@sheetalkamat
Copy link
Copy Markdown
Member

Before when the script info was opened the script info's path was always set using serverHost's current directory. But when this script info gets added to the project that has different current directory (possible when the inferred project is created with projectRoot thats different from the serverhost current directory) the program will always use the path for this script info as currentdirectory + relative name which wont be same as script info's Path. Hence the cause of likes of #19588

Now the script infos opened with relative file name and have the project root, they will be stored in a map so we can access those during lookup correctly. The path for such script info would be created using projectRootPath + fileName instead of using currentDirectory of serverHost.

if (!openedByClient) {
this.watchClosedScriptInfo(info);
}
else if (!isRootedDiskPath(fileName) && currentDirectory !== this.currentDirectory) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.currentDirectory on the editor service as a whole seems wrong to me.. when do you ever want the server directory to resolve a file reference... do not know what to do about it though.. I think the reason it works is that the editors always send us absolute paths. but it just seems wrong

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.. But I think for now we should let it sit that way and later just get rid of this.currentDirectory from projectService and ensure all the file paths are absolute?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also i dont know what to do if non rooted file is opened without providing projectRootPath? which was one of the reason I let it be that way.. (what do we use as current directory then?)
PS: I am updating the current test to run both with and without projectRootPath when opening the file.
Have noticed that if you openFolder in vscode the projectRootPath is sent as that folder path,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just assume that anything that comes from the editor is a Path. do not mess up with it. only closed files, or files that we have generated by resolution should be converted to a Path by us, and for these we always have a directory to resolve the path against.

in other words, if the editor gives us a file name, we should not change it.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Nov 6, 2017

we will need to port this to release-2.6 as well.

@sheetalkamat sheetalkamat merged commit ddbd654 into master Nov 6, 2017
@sheetalkamat sheetalkamat deleted the fileNotOnDisk branch November 6, 2017 20:21
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants