[vscode] Support all workspace.fs APIs for registered fs providers #7824

amiramw · 2020-05-14T21:52:03Z

For this phase only support registered file system providers (and not real file system yet)

Signed-off-by: Amiram Wingarten [email protected]

What it does

Support all missing workspace.fs APIs for registered fs providers: stat, readDirectory, createDirectory, delete, rename and copy.

readFile and writeFile were supported already before this PR.

For the scope of this PR the new supported APIs only work with vscode extensions registered file system providers and not yet support real file system.

How to test

Clone https://github.com/amiramw/vscode-file-system-provider which is a vscode extension that registers inmemory file system provider and exposes commands that test all vscode.workspace.fs methods
yarn
yarn compile
vsce package
Deploy the result vsix file to theia
Test the following commands from command palette:
- FSP: Test vscode.workspace.fs.stat()
- FSP: Test vscode.workspace.fs.readDirectory()
- FSP: Test vscode.workspace.fs.readFile()
- FSP: Test vscode.workspace.fs.createDirectory()
- FSP: Test vscode.workspace.fs.writeFile()
- FSP: Test vscode.workspace.fs.rename()
- FSP: Test vscode.workspace.fs.copy()
- FSP: Test vscode.workspace.fs.delete()

Prior to this PR only readFile and writeFile worked. After this PR all of them should work.

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

akosyakov · 2020-05-15T06:48:38Z

@amiramw I'm rewritting this part now to integrate with Theia FS properly. How urgent is this PR for you? We target to have vscode fs API support as in VS code in 2-3 weeks.

Proposed solution only supports fs providers, not all fs APIs, since disk fs should be integrated properly, plus there are contracts between providers, e.g. when move happens other providers should be notified as well and so on.

amiramw · 2020-05-15T07:52:12Z

Specifically it is urgent for us to support delete and readDirectory for fs providers only.
Is the target design completely different? Can I adjust so at least this limited implementation is aligned with it so you don't have to rewrite later?

I don't mind contributing also the fs support (assuming it is not huge effort) but this PR scope is more urgent.

akosyakov · 2020-05-15T08:16:15Z

Is the target design completely different?

This code will be replaced by code from VS Code extension host, not our own. We don't want to deviate.

I don't mind contributing also the fs support (assuming it is not huge effort) but this PR scope is more urgent.

That's very involving and we want to do it ourself to avoid long PR reviews. We have the same issue with this PR: #7718 (comment)

If such PRs are minimal, i.e. changes don't affect APIs besides the plugin-ext then someone can review and test, otherwise I don't think we need to accept them. That's an effort and these code will be replaced in 2 weeks.

Also such PRs are distraction from brining the complete solution. Please file issues if you want to work on something. I understand that you spent time implementing it, but I had assigned myself for #7269 and #7127 on purpose.

amiramw · 2020-05-15T09:15:23Z

Will these 2 weeks be within the scope of 1.2.0?

akosyakov · 2020-05-15T09:24:18Z

Will these 2 weeks be within the scope of 1.2.0?

I don't think so.

akosyakov · 2020-05-15T09:36:49Z

@amiramw your change is pretty isolated compare to #7718 Maybe someone can review it to merge as a temporary solution. I would prefer to focus on the complete solution.

amiramw · 2020-05-15T10:10:48Z

That would be great 🙏

vince-fugnitto

I verified using the in-memory filesystem extension which you provided and I see the logs in the output when testing each command. I do see errors however when executing the extension (I'm not sure if it is expected):

Uncaught (in promise) Error: fsp:/a.ts
    at new FileSystemError (/Users/vincentfugnit…/types-impl.js:1280)
    at InPluginFileSystemProxy.handleError (/Users/vincentfugnit…system-proxy.js:226)
    at InPluginFileSystemProxy.<anonymous> (/Users/vincentfugnit…system-proxy.js:214)
    at step (/Users/vincentfugnit…esystem-proxy.js:59)
    at Object.throw (/Users/vincentfugnit…esystem-proxy.js:40)
    at rejected (/Users/vincentfugnit…esystem-proxy.js:32)
    at process._tickCallback (/Users/vincentfugnit…ess/next_tick.js:68)
bundle.js:128248 Uncaught (in promise) Error: 'No available file system provider for schema file
    at new FileSystemError (/Users/vincentfugnit…/types-impl.js:1280)
    at InPluginFileSystemProxy.handleError (/Users/vincentfugnit…system-proxy.js:226)
    at InPluginFileSystemProxy.<anonymous> (/Users/vincentfugnit…system-proxy.js:214)
    at step (/Users/vincentfugnit…esystem-proxy.js:59)
    at Object.throw (/Users/vincentfugnit…esystem-proxy.js:40)
    at rejected (/Users/vincentfugnit…esystem-proxy.js:32)
    at process._tickCallback (/Users/vincentfugnit…ess/next_tick.js:68)
bundle.js:128248 Uncaught (in promise) Error: fsp:/copyFile.txt
    at new FileSystemError (/Users/vincentfugnit…/types-impl.js:1280)
    at InPluginFileSystemProxy.handleError (/Users/vincentfugnit…system-proxy.js:226)
    at InPluginFileSystemProxy.<anonymous> (/Users/vincentfugnit…system-proxy.js:214)
    at step (/Users/vincentfugnit…esystem-proxy.js:59)
    at Object.throw (/Users/vincentfugnit…esystem-proxy.js:40)
    at rejected (/Users/vincentfugnit…esystem-proxy.js:32)
    at process._tickCallback (/Users/vincentfugnit…ess/next_tick.js:68)
2
bundle.js:128248 Uncaught (in promise) Error: fsp:/file.txt
    at new FileSystemError (/Users/vincentfugnit…/types-impl.js:1280)
    at InPluginFileSystemProxy.handleError (/Users/vincentfugnit…system-proxy.js:226)
    at InPluginFileSystemProxy.<anonymous> (/Users/vincentfugnit…system-proxy.js:214)
    at step (/Users/vincentfugnit…esystem-proxy.js:59)
    at Object.throw (/Users/vincentfugnit…esystem-proxy.js:40)
    at rejected (/Users/vincentfugnit…esystem-proxy.js:32)
    at process._tickCallback (/Users/vincentfugnit…ess/next_tick.js:68)
bundle.js:128248 Uncaught (in promise) Error: fsp:/file.txt
    at new FileSystemError (/Users/vincentfugnit…/types-impl.js:1280)
    at InPluginFileSystemProxy.handleError (/Users/vincentfugnit…system-proxy.js:226)
    at InPluginFileSystemProxy.<anonymous> (/Users/vincentfugnit…system-proxy.js:197)
    at step (/Users/vincentfugnit…esystem-proxy.js:59)
    at Object.throw (/Users/vincentfugnit…esystem-proxy.js:40)
    at rejected (/Users/vincentfugnit…esystem-proxy.js:32)
    at process._tickCallback (/Users/vincentfugnit…ess/next_tick.js:68)

packages/plugin-ext/src/plugin/file-system.ts

packages/plugin-ext/src/plugin/in-plugin-filesystem-proxy.ts

packages/plugin-ext/src/plugin/types-impl.ts

amiramw · 2020-05-16T21:53:56Z

I verified using the in-memory filesystem extension which you provided and I see the logs in the output when testing each command. I do see errors however when executing the extension (I'm not sure if it is expected):

I don't see any errors when I tests these changes (both locally and on gitpod). Does it reproduce?

amiramw · 2020-05-19T07:04:32Z

@kittaakos @vince-fugnitto any more comments on this PR?

vince-fugnitto · 2020-05-19T13:54:19Z

I verified using the in-memory filesystem extension which you provided and I see the logs in the output when testing each command. I do see errors however when executing the extension (I'm not sure if it is expected):

I don't see any errors when I tests these changes (both locally and on gitpod). Does it reproduce?

@amiramw it is reproducible when running the delete command first (which is likely expected), but I thought that better handling would be present.

amiramw · 2020-05-19T15:31:25Z

I verified using the in-memory filesystem extension which you provided and I see the logs in the output when testing each command. I do see errors however when executing the extension (I'm not sure if it is expected):

I don't see any errors when I tests these changes (both locally and on gitpod). Does it reproduce?

@amiramw it is reproducible when running the delete command first (which is likely expected), but I thought that better handling would be present.

@vince-fugnitto When deleted file does not exist I get: command.ts:15 Uncaught (in promise) EntryNotFound (FileSystemError): fsp:/file.ts
When trying to delete file with file scema I get command.ts:15 Uncaught (in promise) Error: 'No available file system provider for schema file

Do you suggest to handle errors differently? Because I tried to align with the way errors were thrown before in this class, like here: https://github.com/eclipse-theia/theia/pull/7824/files#diff-c908a225bec2806e57d9d1fe1c0ee3c8L70

vince-fugnitto · 2020-05-19T16:01:19Z

@vince-fugnitto When deleted file does not exist I get: command.ts:15 Uncaught (in promise) EntryNotFound (FileSystemError): fsp:/file.ts
When trying to delete file with file scema I get command.ts:15 Uncaught (in promise) Error: 'No available file system provider for schema file

Yes, I noticed the errors when first executing a delete.
Once the first delete happens, executions of other commands result in different thrown errors.
I wanted to confirm if this is the intended behavior or out of scope for the pull-request.

Do you suggest to handle errors differently? Because I tried to align with the way errors were thrown before in this class, like here: https://github.com/eclipse-theia/theia/pull/7824/files#diff-c908a225bec2806e57d9d1fe1c0ee3c8L70

I tried your extension in vscode and the handling was better so I thought it was part of the pull-request as well:

amiramw · 2020-05-24T21:56:37Z

@vince-fugnitto the exception is thrown eventually from the vs code command handler but in theia side such exceptions are ignored.

I don't think that it should be in the scope of this PR.

I wouldn't mind creating another PR and add catch of an exception on:

theia/packages/plugin-ext/src/plugin/command-registry.ts

Line 130 in 18b128c

    
           return handler<T>(...args.map(arg => this.argumentProcessors.reduce((r, p) => p.processArgument(r), arg)));

and print error message.

vince-fugnitto · 2020-05-24T22:21:42Z

@vince-fugnitto the exception is thrown eventually from the vs code command handler but in theia side such exceptions are ignored.

I don't think that it should be in the scope of this PR.

@amiramw no problem, I just wanted to bring up what I noticed during testing.
We do have potential improvements for error handling like you described but I do not feel as though it needs to be covered in this pull-request as you mentioned nor is it a critical issue.

amiramw · 2020-05-24T22:28:24Z

@vince-fugnitto any more comments or can this PR be merged?

vince-fugnitto

@amiramw the changes look good to me , I’ll wait to merge tomorrow unless there are any objections :)

…ders For this phase only support registered file system providers (and not real file system) Signed-off-by: Amiram Wingarten <[email protected]>

amiramw · 2020-05-26T08:29:56Z

@vince-fugnitto can i merge?

vince-fugnitto · 2020-05-26T12:49:15Z

@vince-fugnitto can i merge?

Yes go ahead! I wasn't sure if you have the rights but if you do please go ahead :)

akosyakov · 2020-05-26T12:53:42Z

@amiramw Please could you wait till the current build on master is published and then merge?

amiramw · 2020-05-26T16:21:49Z

@akosyakov is it ok now?

vince-fugnitto · 2020-05-26T16:25:50Z

@akosyakov is it ok now?

Yes, the last commit has finished, CI has completed and it has been deployed 👍

amiramw · 2020-05-26T16:27:48Z

Thank you!

akosyakov added filesystem issues related to the filesystem vscode issues related to VSCode compatibility labels May 15, 2020

akosyakov requested review from RomanNikitenko and vince-fugnitto May 15, 2020 09:36

vince-fugnitto reviewed May 15, 2020

View reviewed changes

kittaakos reviewed May 15, 2020

View reviewed changes

packages/plugin-ext/src/plugin/file-system.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/in-plugin-filesystem-proxy.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved

amiramw force-pushed the fs branch from 5641f3f to 71b5d30 Compare May 16, 2020 21:50

amiramw force-pushed the fs branch from 71b5d30 to 90c4215 Compare May 24, 2020 21:57

vince-fugnitto approved these changes May 24, 2020

View reviewed changes

[vscode] Support all vscode.workspace.fs APIs for registered fs provi…

3e8f728

…ders For this phase only support registered file system providers (and not real file system) Signed-off-by: Amiram Wingarten <[email protected]>

amiramw force-pushed the fs branch from 90c4215 to 3e8f728 Compare May 25, 2020 21:28

amiramw merged commit 84408de into master May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vscode] Support all workspace.fs APIs for registered fs providers #7824

[vscode] Support all workspace.fs APIs for registered fs providers #7824

amiramw commented May 14, 2020 •

edited

Loading

akosyakov commented May 15, 2020 •

edited

Loading

amiramw commented May 15, 2020

akosyakov commented May 15, 2020 •

edited

Loading

amiramw commented May 15, 2020

akosyakov commented May 15, 2020

akosyakov commented May 15, 2020 •

edited

Loading

amiramw commented May 15, 2020

vince-fugnitto left a comment

amiramw commented May 16, 2020

amiramw commented May 19, 2020

vince-fugnitto commented May 19, 2020

amiramw commented May 19, 2020

vince-fugnitto commented May 19, 2020

amiramw commented May 24, 2020

vince-fugnitto commented May 24, 2020

amiramw commented May 24, 2020

vince-fugnitto left a comment

amiramw commented May 26, 2020

vince-fugnitto commented May 26, 2020

akosyakov commented May 26, 2020

amiramw commented May 26, 2020

vince-fugnitto commented May 26, 2020

amiramw commented May 26, 2020

[vscode] Support all workspace.fs APIs for registered fs providers #7824

[vscode] Support all workspace.fs APIs for registered fs providers #7824

Conversation

amiramw commented May 14, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented May 15, 2020 • edited Loading

amiramw commented May 15, 2020

akosyakov commented May 15, 2020 • edited Loading

amiramw commented May 15, 2020

akosyakov commented May 15, 2020

akosyakov commented May 15, 2020 • edited Loading

amiramw commented May 15, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

amiramw commented May 16, 2020

amiramw commented May 19, 2020

vince-fugnitto commented May 19, 2020

amiramw commented May 19, 2020

vince-fugnitto commented May 19, 2020

amiramw commented May 24, 2020

vince-fugnitto commented May 24, 2020

amiramw commented May 24, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

amiramw commented May 26, 2020

vince-fugnitto commented May 26, 2020

akosyakov commented May 26, 2020

amiramw commented May 26, 2020

vince-fugnitto commented May 26, 2020

amiramw commented May 26, 2020

amiramw commented May 14, 2020 •

edited

Loading

akosyakov commented May 15, 2020 •

edited

Loading

akosyakov commented May 15, 2020 •

edited

Loading

akosyakov commented May 15, 2020 •

edited

Loading