feat(vscode): Implements readFile/writeFile for workspace.fs

[benoitf]

What it does

Implements workspace.fsfor readFile and writeFile

How to test

readFile

        const sampleTxtAbsolutePath = context.asAbsolutePath('sample.txt');
	const fileToUse = vscode.Uri.file(sampleTxtAbsolutePath);
	let sampleTextEncoded = await vscode.workspace.fs.readFile(fileToUse);
	let sampleText = new util.TextDecoder('utf-8').decode(sampleTextEncoded);

writeFile

	const sampleTxtToWriteAbsolutePath = context.asAbsolutePath('sampleToWrite.txt');
	const fileToWrite = vscode.Uri.file(sampleTxtToWriteAbsolutePath);
	const contentToWrite = new util.TextEncoder().encode('Hello World');
	await vscode.workspace.fs.writeFile(fileToWrite, contentToWrite);

For other methods like stat, etc, need to revise Theia core stuff as Theia is not providing enough stats info for example

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Change-Id: I1626547e72b8230bbc0a1832f682ebc18f2b22dc
Signed-off-by: Florent Benoit fbenoit@redhat.com

[akosyakov]

@benoitf sorry i could not attend the meeting yesterday, it should be fine to add required metadata to FileStat, @kittaakos ?

[benoitf]

@akosyakov about adding stuff about FileStat during the meeting we agreed to add stuff and deprecate other some methods (shouldn't be a problem)

A wider problem to me is that today we have FileSystem and ResourceProvider/ResourceResolver
new FileSystemProvider was hooked as a Resource (and then we have resources with read/write)

but then, to use createDir, getFileStat, copy/remove and all other methods of workspace.fs
https://github.com/microsoft/vscode/blob/1.39.2/src/vs/vscode.d.ts#L5809-L5874

we won't be able to use Resource API with that design , thoughts ?

[kittaakos]

required metadata

Did you mean mtime, ctime and the file type, such as directory, file, and symlink? Sure, why not. We retrieve the fs.Stats object in the FileSystemNode anyways.

theia/packages/filesystem/src/node/node-filesystem.ts

Line 446 in 3ad2b62

const stats = await fs.stat(FileUri.fsPath(uri));

[benoitf]

@kittaakos yes ctime, mtime, etc but also to have FileType instead of just isDirectory https://github.com/microsoft/vscode/blob/e6651b90f937e9874aeba717cb3724190e01a34f/src/vs/platform/files/common/files.ts#L206-L210

[kittaakos]

have FileType instead of just isDirectory

Nice. I would leave the isDirectory in place, you can depreciate it though so it does not break. Thoughts?

[benoitf]

@kittaakos yes add new stuff + keep it with deprecation

[akosyakov]

we won't be able to use Resource API with that design , thoughts ?

yes, I think in the plugin system to implement workspace.fs, we should use the file system directly then and also contributed file system providers. But we should still hook file system provider as resources that editors could read from them for custom schemes.

I wonder where else we should hook file system providers. Could they contribute somehow to the navigator or VS Code extension supposed to provide own tree views?

[benoitf]

PR updated

[tsmaeder]

I'm worried about this line: will it provide the exact byte contents of the file in every case? Reading the file as a string will use a certain text encoding. How can we be sure we're using the same encoding on this line?

[benoitf]

@tsmaeder because it is using utf-8 ?

[tsmaeder]

But you don't know that: resources are a point of extension. Say a resource reads a file in EBDIC and translates that correctly to a string. When you encode that using utf-8, you will not have the original byte contents in your byte array.

[benoitf]

yes, it was like temporary issue (for non utf-8 resources) until being able to use filesystem from workspace.fs (and then hook FileStat etc in theia core)
For now we only have resources allowing the file access + custom FileSystemResourceProvider

[tsmaeder]

we won't be able to use Resource API with that design , thoughts ?

I think this is correct: Resource is the interface between the editor and content: FileSystem is the interface between storage and content.
Every file is a resource, but not every resource is a file. Also: files deal with bytes, resources with strings. Encoding needs to happen in between.

[tsmaeder]

Also, I was wondering why we need this now. Is there an issue this fixes or a particular VS Code extension it enables?

[benoitf]

Related VS Code extension: https://github.com/microsoft/vscode-extension-samples/blob/master/call-hierarchy-sample/src/extension.ts#L21

[akosyakov]

@tsmaeder Could you finish the review please?

[benoitf]

@tsmaeder ?

[tsmaeder]

I don't think we should merge this PR in the current form. The architectural problem that files are implemented in terms of resources leads to potential problems with encoding. I would agree to this workaround if there was a clear business reason to do it, but not to enable an example plugin (that's why I asked what it's for). Let's do this right! I might be swayed if there is a clear commitment to fix the issues soon.

[benoitf]

well, I would argue that there are other extensions using that
https://github.com/search?q=%22vscode.workspace.fs.readFile%22&type=Code

and from what I've seen, they're all using UTF-8.

but well, it's up to reviewers.

[tsmaeder]

I'm all for it, but let's do it right. Would it be so hard to fix the resource/file issue?

[benoitf]

I just wanted to proceed by steps

1. introduce readFile/writeFile
2. introduce stat/type API in theia core
3. Hook filesystemproviders into theia filesystem
4. Map fs API to theia filesystem

but of course we can omit step 1, just that it was easy to provide it like this without any changes yet in theia core.

[tsmaeder]

Why don't you open an issue for this so we can track it? Also, is there a way to test this in real life?

[benoitf]

what are you calling 'real life' ?
I will add issue

Also it has been discussed during theia dev meeting a couple of weeks ago

[tsmaeder]

Is there a plugin I can install and exercise the code?

[benoitf]

@tsmaeder so by looking at my github search link [1] it turns out that markdown language server is using the API [2]

And this extension is provided by default in Eclipse Theia.

[1] https://github.com/search?q=%22vscode.workspace.fs.readFile%22&type=Code
[2] https://github.com/microsoft/vscode/blob/master/extensions/markdown-language-features/src/features/workspaceSymbolProvider.ts#L89-L90

[akosyakov]

I think we can accept it, some small extension would be helpful to drop and verify? Or how you check it?

[benoitf]

How to check with callhierachy sample: (I haven't looked at how markdown LS is using readFile api)

start Theia with pre-build vsix on a gist from examples/browser directory

$ THEIA_PLUGINS=https://gist.github.com/benoitf/234f32b79ce621b7a6a2f6b2c13ddd24/raw/c796ba66654d3bdb86f70ff7d420e495e83314f2/call-hierarchy-sample-0.0.1.vsix  node src-gen/backend/main.js

Then create a dummy foo.txt file (to trigger actionEvent on the sample)
It will then create a new Editor based on a file read from the VS Code extension

[akosyakov]

it works nicely for me

@tsmaeder could we merge it for now?

[akosyakov]

introduce readFile/writeFile
introduce stat/type API in theia core
Hook filesystemproviders into theia filesystem
Map fs API to theia filesystem

Regarding steps It is a bit unclear to me why do we need to hook plugin fs providers in core filesystem. Sorry i did not research how these providers should be used. But if they are need only for VS Code compatibility I think we can leave core filesystem as is and build something on top like a facade to core filesystem and plugin fs providers.

[tsmaeder]

@benoitf When I follow your testing instructions with with the call-hierarchy sample, a breakpoint in languages-main.ts#readFile is not hit.

[tsmaeder]

@benoitf When I follow your testing instructions with with the call-hierarchy sample, a breakpoint in languages-main.ts#readFile is not hit.

Stupid old me is using the "fixed" version you provided for the call hieararchy PR 🤦‍♂

[tsmaeder]

Hmh...still can't hit that breakpoint. I guess you have @akosyakov's approval, so you don't need mine.

[benoitf]

ok, I will merge today if no objections