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

Experiment: remote file system #30337

Merged
merged 8 commits into from
Jul 21, 2017
Merged

Experiment: remote file system #30337

merged 8 commits into from
Jul 21, 2017

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Jul 10, 2017

This is an experiment about allowing extensions to contribute something like a remote file system to VS Code. For now, only loading and storing is tackled, no discovery nor creation of files/folders. Also, only text files are considered for now, no binaries etc.

To try this have an extension (using proposed API*) that does the following:

    vscode.workspace.registerFileSystemProvider('foo', {
        onDidChange: new vscode.EventEmitter<vscode.Uri>().event,
        resolveContents(resource) {
            return `It's ${Date.now().toString(23)} o'clock`;
        },
        writeContents(resource, value) {
            // pretending this happened...
            return;
        }
    })

jul-10-2017 16-27-18

*) Enable proposed API by having this line in your package.json "enableProposedApi": true, and for IntelliSense copy the most recent version of vscode.proposed.d.ts to your extension.

@mention-bot
Copy link

@jrieken, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @egamma to be potential reviewers.

@jrieken jrieken self-assigned this Jul 10, 2017
@egamma egamma changed the title Experiment: remove file system Experiment: remote file system Jul 10, 2017
@kieferrm kieferrm mentioned this pull request Jul 11, 2017
39 tasks
bolinfest added a commit to bolinfest/vs-code-preview-html that referenced this pull request Jul 11, 2017
This is built on top of microsoft/vscode#30337.

Note that I had to follow the instructions at
microsoft/vscode#13394 to be able to run
this extension from the OSS version of VS Code.
Unfortunately, that means I made a personal modification to
`.vscode/launch.json`, so hopefully I can find some way to avoid that.

This is pretty exciting in that I had to make minimal changes to my
existing extension to get this to work! The only issue that I'm running
into right now is that `resolveContents()` appears to be called more
often than I expect. It is called twice when I open the file for the
first time. I also opened a remote `.md` file using this mechanism. I was
able to use `Markdown: Open Preview` on this remote file, but when I
closed the preview, `resolveContents()` was called again and I'm not sure
why. Because loading remote file contents could be expensive, we would
ideally fix things so this is called sparingly, or we'll have to maintain
a local cache to avoid extra roundtrips.

Note that I had to launch the OSS version (built from source) using
`./scripts/code.sh --remote nuclide`. There is a TODO in the PR to
eliminate the need to pass this flag.
@bolinfest
Copy link
Contributor

Good news! I got this working with my existing demo with very few code changes: bolinfest/vs-code-preview-html@4e7647b

As you can see from the commit message for that commit, I'm seeing resolveContents() getting called additional times. Otherwise, things seem to be working as expected.

I'm also excited to see that both methods can return a Thenable, though I didn't try returning errors from resolveContents or writeContents yet to see how those were surfaced in the UI.

I also haven't tried wiring up any remote LSPs yet. I may not be able to do that until Wednesday.

@jrieken
Copy link
Member Author

jrieken commented Jul 11, 2017

FYI - I have removed the need for the --remote-flag, having this behind proposed API should be enough protection. I have updated the description accordingly.

I'm seeing resolveContents() getting called additional times. Otherwise, things seem to be working as expected.

Can you provide a little more detail? Today, we throw things away when the last editor or tab closes, when then opening a file again we go back the truth. Is that what you are seeing or are there other unexpected calls?

I'm also excited to see that both methods can return a Thenable, though I didn't try returning errors from resolveContents or writeContents yet to see how those were surfaced in the UI.

Yeah, error handling should be working (tho is untested 🙈). Also, as suggested we should look into some sort of streaming API that enables providers to deliver a file in chunks. That and a long list of other features is to be defined, let's for now focus on the MVP

@bolinfest
Copy link
Contributor

@jrieken Are you sure things are still working now that you dropped the --remote thing? I rebuilt and now I can't get my extension to work anymore.

@bolinfest
Copy link
Contributor

@jrieken Yep, I went back to what you had before (the top of the stack of two commits) and rebuilt:

git checkout 16c00672fad01309cfbb4a6caa7674fc04feba81
./scripts/npm.sh install

and then I launched with ./scripts/code.sh --remote nuclide and things worked again.

@bolinfest
Copy link
Contributor

Also, I had to add this to vscode.proposed.d.ts when I added that file to the typings/custom folder in my extension (I don't know if that's the right place to put it, but that's what someone suggested):

import { Event, Uri } from "vscode/vscode";

I also got an error on readonly name: string; in Terminal, so I just commented out Terminal altogether for now.

@jrieken
Copy link
Member Author

jrieken commented Jul 13, 2017

Are you sure things are still working now that you dropped the --remote thing? I rebuilt and now I can't get my extension to work anymore.

Yeah, pretty sure. I have a simple sample here and that still works. Do you see any errors or does the debugger break on any errors?

Also, I had to add this to vscode.proposed.d.ts when I added that file to the typings/custom folder in my extension (I don't know if that's the right place to put it, but that's what someone suggested):

I usually just put it next to my extension sources, tho it shouldn't really matter as long as there is a "recent" version of vscode.d.ts and a recent version of TypeScript. The definitions of both files should be merged. What error are you seeing in the terminal?

@siegebell
Copy link

The errors in vscode.proposed.d.ts are a result of an out-of-date vscode engine version; updating this will fix the issue.

@bolinfest
Copy link
Contributor

FYI, I updated to c510eff and did a clean rebuild and everything looks good!

@jrieken jrieken added this to the July 2017 milestone Jul 20, 2017
@jrieken jrieken merged commit 1369a43 into master Jul 21, 2017
@jrieken
Copy link
Member Author

jrieken commented Jul 21, 2017

Merging onto master such that it can be tried out easier.

@jrieken jrieken deleted the joh/remote branch July 21, 2017 08:19
@felixfbecker
Copy link
Contributor

felixfbecker commented Jul 22, 2017

@jrieken as I understand it, this selects the correct provider based on the URI authority (host and port), correct? Wouldn't it make more sense to select it based on the URI scheme? E.g. have one handler for file: URIs, one for ftp:, etc.

@jrieken
Copy link
Member Author

jrieken commented Jul 24, 2017

Wouldn't it make more sense to select it based on the URI scheme?

Totally! Just a lot harder to implement because for some reason our code makes a ton of assumptions on the file and untitled schemes. Once this becomes real it will be based on scheme

@sqs
Copy link

sqs commented Jul 26, 2017

It would be nice if extensions could specify in activationEvents that they should be loaded in order to provide the file system for a given authority. I have a POC of this working (with a few other changes, such as routing on scheme not authority, but you can just ignore those):

https://gist.github.com/sqs/3c8dc223d5d418f09cec4a68020d9038#file-remotefileservice-ts-L98

This requires the extension API FileSystemProvider to always return promises (never just string).

We are trying out this new API and will hopefully have more patches/experiences to send along. So far, it's great. We've moved all of our remote file system code to an extension.

@jrieken
Copy link
Member Author

jrieken commented Jul 26, 2017

Totally, the scheme-activation event has been already discussed for the virtual documents api and will be part of this, no doubt about that.

@siegebell
Copy link

Reopening tabs upon vscode restart needs to work with remote files.

I think this also needs schema activation events -- so that vscode can wait for the associated extension to activate, giving it a change to register a file system, before requesting that the remote file be loaded.

@bolinfest
Copy link
Contributor

bolinfest commented Jul 27, 2017 via email

@jrieken
Copy link
Member Author

jrieken commented Jul 28, 2017

Could we get some sort of "authority activation event" for now?

Unsure, while this is experimental I'd go with the * event. Reopening files is on deck, in general files from remote source should "feel" no different that local files.

@dlebu
Copy link

dlebu commented Sep 5, 2017

@jrieken I am trying to launch VSCode from a webpage using the URL mechanism described here, and open a remote file system automatically. Is this expected to work given the current state of the remote file system implementation?
If not, what would be a good way to achieve that?

@jrieken
Copy link
Member Author

jrieken commented Sep 6, 2017

Is this expected to work given the current state of the remote file system implementation?

No and unsure how we could make it work today...

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

8 participants