[api] Improve callback FS - #64447
Andrew Branch (andrewbranch) wants to merge 9 commits into
Conversation
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? |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is coherent and tested; only two non-blocking test comments misname fakeStat.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Improves API filesystem callbacks by requiring explicit implementations and adding server-side sentinels.
Changes:
- Adds
serverFSsentinels, 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.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
|
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
Can you clarify what you mean? |
|
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? |
|
Yes, exactly. The idea is if you can name a well-known server implementation, you can save a lot of round trips. |
| 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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Though, we do we need this now?
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <[email protected]>

Prior to #64115, we had
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 supplyreadFilebut forget to supplyfileExists.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
createVirtualFileSystem(or rather, moves it to test utilities so we can keep relying on it for testing)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
readFileonly, and let everything else use the OS, previously I would have specified onlyreadFile. Now, that looks like this:Every key is required, so if we ever expand the interface, you'll get notified with a type error. (
serverFS.useOSis a unique symbol sentinel, not an actual callback.)Other
serverFSfunctions includeserverFS.errorwhich 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 FSwriteFilesupportsserverFS.noopstatsupportsserverFS.fakeStat(infers existence/mode fromfileExistsanddirectoryExists)realpathsupportsserverFS.identity(the identity function)Those symbols can also be returned from callbacks to delegate control back to the server: