Skip to content

[api] Improve callback FS - #64447

Open
Andrew Branch (andrewbranch) wants to merge 9 commits into
microsoft:mainfrom
andrewbranch:api-improve-callback-fs
Open

Andrew Branch (andrewbranch) wants to merge 9 commits into
microsoft:mainfrom
andrewbranch:api-improve-callback-fs

Conversation

@andrewbranch

Copy link
Copy Markdown
Member

Prior to #64115, we had

const api = new API({
  fs: createVirtualFileSystem({
    "/tsconfig.json": "{}"
  })
});

which worked by registering callbacks for readFile, fileExists, getAccessibleEntries, etc. for the server to call back to the client with.

One piece of feedback that surfaced with the callbacks was that if you supplied your own, rather than using createVirtualFileSystem, any callbacks you omitted silently resulted in the server using the OS file system for those callbacks, making it easy to create an inconsistency where you supply readFile but forget to supply fileExists.

Additionally, now that we have #64115, file system callbacks might still be useful, but they shouldn't be used to serve up a complete in-memory file system. Using #64115 accomplishes the same thing with much better performance, since the server avoids a round trip per FS call.

So, this PR

  • removes createVirtualFileSystem (or rather, moves it to test utilities so we can keep relying on it for testing)
  • changes the callback-based file system to enforce specifying an implementation for every function.

You don't have to implement a callback for every function—there are symbols that represent a few possible server-side implementations. For example, if I want to override readFile only, and let everything else use the OS, previously I would have specified only readFile. Now, that looks like this:

import { API } from "typescript/unstable/sync";
import { serverFS } from "typescript/unstable/fs";

const api = new API({
  fs: {
    readFile: customReadFileImplementation,
    fileExists: serverFS.useOS,
    directoryExists: serverFS.useOS,
    getAccessibleEntries: serverFS.useOS,
    stat: serverFS.useOS,
    realpath: serverFS.useOS,
    writeFile: serverFS.useOS,
  },
});

Every key is required, so if we ever expand the interface, you'll get notified with a type error. (serverFS.useOS is a unique symbol sentinel, not an actual callback.)

Other serverFS functions include

  • every callback supports serverFS.error which panics (recovered, try/catchable from the client)—useful if you've set up a full in-memory snapshot file system and want to assert to yourself that you're never accidentally touching the real FS
  • writeFile supports serverFS.noop
  • stat supports serverFS.fakeStat (infers existence/mode from fileExists and directoryExists)
  • realpath supports serverFS.identity (the identity function)

Those symbols can also be returned from callbacks to delegate control back to the server:

const api = new API({
  fs: {
    readFile: fileName => {
      if (shouldOverride(fileName)) {
        return overrideFileContents[fileName];
      }
      return serverFS.useOS;
    },
    // ...
  },
});

Copilot AI balanced review requested due to automatic review settings September 25, 2026 17:36
@github-project-automation github-project-automation Bot moved this to Not started in PR Backlog Sep 25, 2026
@typescript-automation typescript-automation Bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 25, 2026
@jakebailey

Copy link
Copy Markdown
Member

Every key is required, so if we ever expand the interface, you'll get notified with a type error.

Historically this means we can't change it, or it's annoying (I remember strada needing realpath but it being optional in the host and then we had to work around it).

Is there some way to provide something where people can really just inspect the calls they care about too?

Copilot AI left a comment

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.

Copilot review overview

🟢 Approval recommended

The implementation is coherent and tested; only two non-blocking test comments misname fakeStat.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
What changed in this PR

Improves API filesystem callbacks by requiring explicit implementations and adding server-side sentinels.

Changes:

  • Adds serverFS sentinels, stat callbacks, symlink metadata, and case-sensitivity configuration.
  • Moves the virtual filesystem helper to test utilities.
  • Updates sync/async clients and tests for the new protocol.
File Description
tsc/​internal/​api/​server.go Propagates case sensitivity.
tsc/​internal/​api/​callbackfs.go Implements new callback protocol.
tsc/​internal/​api/​callbackfs_test.go Tests callback behavior.
tsc/​cmd/​tsc/​api.go Adds callback and case flags.
packages/​typescript/​test/​testUtils.ts Hosts the test-only VFS.
packages/​typescript/​test/​sync/​astnav.test.ts Updates VFS import.
packages/​typescript/​test/​sync/​ast.test.ts Updates VFS import.
packages/​typescript/​test/​sync/​ast.bench.ts Updates benchmark VFS import.
packages/​typescript/​test/​sync/​api.testUtils.ts Updates VFS import.
packages/​typescript/​test/​sync/​api.test.ts Tests sync callback semantics.
packages/​typescript/​test/​diagnosticFormatter.test.ts Updates VFS import.
packages/​typescript/​test/​async/​astnav.test.ts Updates VFS import.
packages/​typescript/​test/​async/​api.testUtils.ts Updates VFS import.
packages/​typescript/​test/​async/​api.test.ts Tests async callback semantics.
packages/​typescript/​src/​api/​sync/​client.ts Encodes sync callbacks.
packages/​typescript/​src/​api/​options.ts Adds case-sensitivity options.
packages/​typescript/​src/​api/​fsCallbacks.ts Configures and validates callbacks.
packages/​typescript/​src/​api/​fs.ts Defines callback and sentinel APIs.
packages/​typescript/​src/​api/​async/​client.ts Encodes async callbacks.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/typescript/test/async/api.test.ts Outdated
Comment thread packages/typescript/test/sync/api.test.ts Outdated
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
@andrewbranch

Copy link
Copy Markdown
Member Author

Well, I think the reality is going to be that we can't change it anyway, until an npm major version bump. Currently all the keys are required, and I expect if we added any in/before 8.0, we would make them required in 8.0. If we wanted to add any before that, they would have to be optional, and we would have to reckon with what that means. So like, if we didn't have stat today and needed to add it, it would be reasonable to make it default to fakeStat, stubbing out the implementation from other specified callbacks. But we wouldn't be able to add a new function and make it default to useOS. We only have room to stub implementations from other specified functions or avoid calling it altogether. I think everything else would be a SemVer break.

Is there some way to provide something where people can really just inspect the calls they care about too?

Can you clarify what you mean?

@DanielRosenwasser

Copy link
Copy Markdown
Member

Is there a reason why you delegate to a symbol instead of providing the "native" function? Is it because that'd involve an extra back-and-forth in sending the same arguments to the API server?

@andrewbranch

Copy link
Copy Markdown
Member Author

Yes, exactly. The idea is if you can name a well-known server implementation, you can save a lot of round trips.

Comment thread tsc/cmd/tsc/api.go
flags.StringVar(&result.pipePath, "pipe", "", "use named pipe or Unix domain socket for communication instead of stdio")
flags.StringVar(&result.callbacks, "callbacks", "", "comma-separated list of FS callbacks to enable (readFile,fileExists,directoryExists,getAccessibleEntries,realpath)")
flags.StringVar(&result.callbacks, "callbacks", "", "comma-separated list of FS callbacks and defaults to enable")
flags.BoolVar(&result.caseSensitive, "useCaseSensitiveFileNames", osvfs.FS().UseCaseSensitiveFileNames(), "treat filesystem paths as case-sensitive")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume this is "internal" in that only us call it and there's no warranty on it? My typed path PR changes this terminology of course.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Though, we do we need this now?

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.

In Strada, it's possible to control the case sensitivity of everything regardless of your OS's case sensitivity. If you're implementing a fully virtual file system in the API, you should be able to control that yourself. So, it's not internal and I think we do need it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

4 participants