tsserver support for a virtual filesystem #48497

sandersn · 2022-03-31T15:46:00Z

I moved the virtual file system with watch code to a new project. The server now depends on this new project. The split might not be perfect: there may still be some test code in the new project.
I'm not at all sure that the way of starting the tsserver in vfs mode is right, and I don't know how to test it.
The test of the new server message still needs to move to a separate file.

Partly fixes #47600 -- there is probably more work needed here and definitely in vs code.

Except for the new test I added.

I should have done this as two commits. Sorry.

I'm pretty sure I need to delete and re-number some of the rest though.

Plus fix associated mistakes in the code found by the test.

Shortly to be undone, but whatever.

Doesn't quite build yet, uploading so I can use github to view the diff.

But editorServices needs some fixes. At least, createDirectory needs a recursive version.

Next: Make a way to create a vfs server from the start.

typescript-bot · 2022-03-31T15:46:03Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

sheetalkamat · 2022-03-31T19:07:59Z

src/server/editorServices.ts

+                let it;
+                while (!(it = createdFiles.next()).done) {
+                    const document = it.value;
+                    if (document.fileContent) {


I think you can do fs.ensureFileOrFolder?

sheetalkamat · 2022-03-31T19:07:59Z

src/server/editorServices.ts

+            if (deletedFiles) {
+                // TODO: Probably want to delete empty parent folders while they are empty too
+                for (const file of deletedFiles) {
+                    fs.deleteFile(file);


removeFileOrFolder has option to determine if its leaf node that you can use.

sheetalkamat · 2022-03-31T19:07:59Z

src/server/editorServices.ts

+                        if (fs.fileExists(it.value.file)) {
+                            fs.modifyFile(it.value.file, it.value.fileContent);
+                        }
+                        else {


It shouldnt be the case right.. we can just call modifyFiles.. So we dont have to check if file exists or not

sheetalkamat · 2022-03-31T19:07:59Z

src/server/protocol.ts

+         * Used to specify the script kind of the file explicitly. It could be one of the following:
+         *      "TS", "JS", "TSX", "JSX"
+         */
+        scriptKindName?: ScriptKindName;


Whats the use of this

sheetalkamat · 2022-03-31T19:07:59Z

src/server/protocol.ts

+        updated: FileSystemRequestArgs[];
+    }
+
+    export interface FileSystemRequestArgs extends FileRequestArgs {


I think you should just create this without deriving from FileRequestArgs as we just need fileName and fileContent?

Presumably, it should resemble OpenRequestArgs.

src/harness/virtualFileSystemWithWatch.ts

sheetalkamat · 2022-03-31T19:07:59Z

src/testRunner/unittests/tsserver/applyChangesToOpenFiles.ts

+        // TODO: This is almost certainly in harness/virtualFileSystemHost.ts, or should be.
+        function verifyFileSystem(host: TestFSWithWatch.VirtualServerHost | undefined, files: protocol.FileSystemRequestArgs[]) {
+            // 1. make sure that everything in files is there
+            assert.isDefined(host);


I think instead of this you probably want to use baselines.. this becomes hard to maintain later on..

const session = createSession(host, { canUseEvents: true, logger: createLoggerWithInMemoryLogs() }); // You can diff the vfs using vfs.snap and vfs.diff to add to the baseline // Do your things here baselineTsserverLogs("configuredProjects", "when multiple projects are open detects correct default project", session);

sheetalkamat · 2022-03-31T19:07:59Z

tests/baselines/reference/types.asyncGenerators.es2018.2.errors.txt

@@ -168,13 +168,13 @@ tests/cases/conformance/types/asyncGenerators/types.asyncGenerators.es2018.2.ts(
    async function * explicitReturnType10(): IterableIterator<number> {
                                             ~~~~~~~~~~~~~~~~~~~~~~~~
 !!! error TS2741: Property '[Symbol.iterator]' is missing in type 'AsyncGenerator<number, any, undefined>' but required in type 'IterableIterator<number>'.
-!!! related TS2728 /.ts/lib.es2015.iterable.d.ts:55:5: '[Symbol.iterator]' is declared here.
+!!! related TS2728 /.ts/lib.es2015.iterable.d.ts:90:5: '[Symbol.iterator]' is declared here.


How come this changed?

sheetalkamat · 2022-03-31T19:07:59Z

src/tsserver/webServer.ts

@@ -120,4 +145,28 @@ namespace ts.server {
        // Start listening
        session.listen();
    }
+
+    function startVirtualFileSystemSession(options: StartSessionOptions, logger: Logger, cancellationToken: ServerCancellationToken) {


I think instead of this you would want to create session with optional option fsSystem which defaults to host. And then in Editor services and session you want to verify all FS events or on the FS host.. So we dont need two sessions and this can be done from any type of Session.

When testing you would want to send the VFS instance and test few operations that use FS.

VFS instance should the host project and services use but server host is used for everything else.. So GC and other things work as before

And then --vfs options can be parsed in both NodeServer as well as WebServer

sheetalkamat · 2022-03-31T19:07:59Z

src/vfs/tsconfig.json

+        { "path": "../jsTyping" },
+        { "path": "../services" }


You probably want to strip down so that you are not depending on these but its not important as anyways this is used in server which uses these dependencies

+1, it's not at all clear why a file system would depend on these.

I'm not sure why this fails locally, I suspect I need to `npm ci`.

mjbvz

Thanks for looking into this @sandersn!

I looked over the protocol part of the change. One question I have is how we'd tell TS which file system a given file belongs to. Would this be part of the filename we give to TS?

amcasey · 2022-04-01T22:40:59Z

    const sys = server.createWebSystem(webHost, args, () => findArgument("--executingFilePath") || location + "");

If we're already reading arguments here, I'd probably just check for --vfs inside createWebSystem, rather than creating a new entrypoint.

Refers to: src/tsserver/webServer.ts:88 in f67ef37. [](commit_id = f67ef37, deletion_comment = False)

amcasey

I basically just second everything @sheetalkamat said 😄

amcasey · 2022-04-01T22:42:58Z

src/vfs/tsconfig.json

+        { "path": "../jsTyping" },
+        { "path": "../services" }


+1, it's not at all clear why a file system would depend on these.

amcasey · 2022-04-01T22:42:58Z

src/vfs/tsconfig.json

+    "compilerOptions": {
+        "outFile": "../../built/local/vfs.js",
+        "types": [
+            "node", "mocha", "chai"


I don't think we want product code to depend on test frameworks?

amcasey · 2022-04-01T22:42:58Z

src/server/protocol.ts

+        updated: FileSystemRequestArgs[];
+    }
+
+    export interface FileSystemRequestArgs extends FileRequestArgs {


Presumably, it should resemble OpenRequestArgs.

amcasey · 2022-04-01T22:42:58Z

src/server/protocol.ts

+        /** For now, only 'memfs', initially for exclusive in-memory operation, but it could be other in-memory names later */
+        fileSystem: string;
+        /** List of newly created or newly available files. */
+        created: FileSystemRequestArgs[];


Is it an error to create a file that already exists? If not, what's the point of having a separate updated property? Do those files provide diffs?

amcasey · 2022-04-01T22:42:58Z

src/server/session.ts

+            [CommandNames.UpdateFileSystem]: (request: protocol.UpdateFileSystemRequest) => {
+                this.changeSeq++;
+                this.projectService.updateFileSystem(
+                    request.arguments.created && arrayIterator(request.arguments.created),


Why are we turning the arrays into iterators?

(It looks like you're following a precedent, so I'm probably missing something.)

amcasey · 2022-04-01T22:42:58Z

src/tsserver/server.ts

@@ -79,6 +79,16 @@ namespace ts.server {
    if (typeof process !== "undefined") {
        start(initializeNodeSystem(), require("os").platform());
    }
+    // TODO: Not sure this is right
+    else if (findArgument("vfs")) {


hasArgument?

Also, I don't know if we validate command line arguments but, if we do, we should probably warn/error if you pass this to a node-based server.

amcasey · 2022-04-01T22:42:58Z

src/tsserver/webServer.ts

@@ -81,6 +93,19 @@ namespace ts.server {
        }
    }

+
+    function createVirtualFileSystem() {


Nit: consider inlining this or, at least, moving it closer to the only caller.

amcasey · 2022-04-01T22:42:58Z

src/tsserver/webServer.ts

+        Debug.assert(sys === undefined);
+        // TODO: I don't think I need the XMLHttpRequest code since vfs will get its files from messages sent by the owner
+        // ...but this means I may not need the webSession-copied code in startVirtualFileSystemSession
+        const vfshost = TestFSWithWatch.createVirtualServerHost([]);


We should probably rename TestFSWithWatch.

How does it relate to server.createWebSystem? Does one delegate to the other?

Project, editor services, session, etc all take an optional fshost and preferentially use it for filesystem operations.

sandersn added 20 commits Mar 1, 2022

initial scribbles

da2622c

create vfs project

9e0e980

Everything builds and tests pass

eb22b4f

Except for the new test I added.

Split fakesHosts and some utils into vfs.

6afd939

I should have done this as two commits. Sorry.

Try to make test behave same as existing one

91f47f7

Test gets further than before

6224ddc

I'm pretty sure I need to delete and re-number some of the rest though.

Test is now passing

160490c

New test better tests UpdateFileSystem

287b6a5

Plus fix associated mistakes in the code found by the test.

try to add a step to test (it fails)

afecf63

Merge branch 'main' into server-vfs-support

587ac14

output vfs in tsserver.js

24a8ba4

Merge branch 'main' into server-vfs-support

c4eb15c

Copy file watching from virtualFileSystemWithWatch

85eee48

Shortly to be undone, but whatever.

Switch to virtualFileSystemWithWatch, move other files back

050e058

Doesn't quite build yet, uploading so I can use github to view the diff.

Tests build and run

978c0bc

But editorServices needs some fixes. At least, createDirectory needs a recursive version.

Tests pass again

a1b9983

Next: Make a way to create a vfs server from the start.

Merge branch 'main' into server-vfs-support

197308e

Add tsserver entrypoint for vfs

22c9470

fix lint

11717fe

clean up oodate TODOs

bb00916

typescript-bot assigned sandersn Mar 31, 2022

typescript-bot added the Author: Team label Mar 31, 2022

sandersn requested a review from sheetalkamat Mar 31, 2022

typescript-bot added the For Milestone Bug label Mar 31, 2022

sandersn added 2 commits Mar 31, 2022

update cleaned-up API baseline

6f599af

Merge branch 'main' into server-vfs-support

73adea3

sheetalkamat requested changes Mar 31, 2022

View changes

sandersn added 2 commits Mar 31, 2022

Try manually reverting failing baseline

1488440

I'm not sure why this fails locally, I suspect I need to `npm ci`.

revert added newline from unix editor

f67ef37

mjbvz reviewed Apr 1, 2022

View changes

move vfs test code vfs->harness

5e0b517

amcasey reviewed Apr 1, 2022

View changes

sandersn added 2 commits Apr 2, 2022

restore order of old code a bit

d0ad777

Merge branch 'main' into server-vfs-support

bacc170

sandersn added this to Not started in PR Backlog Apr 8, 2022

sandersn moved this from Not started to Waiting on reviewers in PR Backlog Apr 8, 2022

Thread fshost through server

b178a3c

Project, editor services, session, etc all take an optional fshost and preferentially use it for filesystem operations.

Mar	APR	May
	11
2021	2022	2023

microsoft / TypeScript Public

tsserver support for a virtual filesystem #48497

tsserver support for a virtual filesystem #48497

sandersn commented Mar 31, 2022

typescript-bot commented Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

amcasey Apr 1, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

sheetalkamat Mar 31, 2022

amcasey Apr 1, 2022

mjbvz left a comment

amcasey commented Apr 1, 2022

amcasey left a comment

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

amcasey Apr 1, 2022

microsoft / TypeScript Public

tsserver support for a virtual filesystem #48497

Are you sure you want to change the base?

tsserver support for a virtual filesystem #48497

Conversation

sandersn commented Mar 31, 2022

typescript-bot commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjbvz left a comment

amcasey commented Apr 1, 2022

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment