Add optional VFS parameters to updateSnapshot - #64115
Add optional VFS parameters to updateSnapshot#64115Wesley Wigham (weswigham) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds snapshot-scoped memory/cache virtual filesystems, incremental snapshot updates, snapshot-based program creation, and memory-backed emit results.
Changes:
- Implements layered VFS support with symlinks and removed paths.
- Extends sync/async APIs for snapshot updates and program creation.
- Adds comprehensive Go and TypeScript API tests.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tsc/internal/vfs/cachedvfs/cachedvfs.go |
Exposes wrapped filesystems. |
tsc/internal/project/snapshot.go |
Carries filesystem overrides across snapshots. |
tsc/internal/project/refcountcache_test.go |
Tests override retention. |
tsc/internal/project/api.go |
Passes VFS state into snapshot operations. |
tsc/internal/api/snapshotfilesystem.go |
Implements memory/cache VFS behavior. |
tsc/internal/api/snapshotfilesystem_test.go |
Tests VFS and snapshot integration. |
tsc/internal/api/session.go |
Integrates VFS updates, programs, and emit. |
tsc/internal/api/session_createprogram_test.go |
Tests snapshot-based program creation. |
tsc/internal/api/proto.go |
Defines Go protocol types. |
packages/typescript/test/sync/api.test.ts |
Tests synchronous VFS APIs. |
packages/typescript/test/sync/api-generators.test.ts |
Tests generator parity. |
packages/typescript/test/async/api.test.ts |
Tests asynchronous VFS APIs. |
packages/typescript/src/api/sync/types.ts |
Exposes emit filesystem results. |
packages/typescript/src/api/sync/api.ts |
Implements synchronous snapshot APIs. |
packages/typescript/src/api/proto.ts |
Adapts snapshot protocol requests. |
packages/typescript/src/api/proto.generated.ts |
Adds generated VFS protocol types. |
packages/typescript/src/api/path.ts |
Decodes UNC URI paths. |
packages/typescript/src/api/fs.ts |
Adds snapshot filesystem factories. |
packages/typescript/src/api/async/types.ts |
Exposes async emit filesystem results. |
packages/typescript/src/api/async/client.ts |
Deduplicates concurrent connections. |
packages/typescript/src/api/async/api.ts |
Implements asynchronous snapshot APIs. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
This looks really cool!
I was previously imagining this would fully replace |
| } | ||
|
|
||
| /** @internal */ | ||
| async createProgramFromSnapshot( |
There was a problem hiding this comment.
I'm a bit confused as to why we want this...
oldProgram is already supposed to be the way to provide a base snapshot, indirectly. At the very least we'd have to make sure oldProgram's snapshot is the same as baseSnapshot, but I still find it confusing to provide both.
I thought that if you were using createProgram, you weren't supposed to really care about snapshots.
There was a problem hiding this comment.
I'm guessing because this is the only way you can use createProgram with one of these file systems... maybe there's a solution here where the existing api.createProgram signature takes a fileSystem?
There was a problem hiding this comment.
I could swap to that, but then would need to feed it through the createProgram protocol and manage snapshot creation on the backend, basically duplicating what the updateSnapshot handler already does. Plus, while today,
snapshot.createProgram(...) is strictly going to narrow the scope of the snapshot (to just the program), I imagine when we lift the restriction on there only being one real snapshot, snapshot.createProgram(...) is going to feel a lot more natural for, eg, making 20 programs for build mode out of a single input snapshot.
There was a problem hiding this comment.
I'm still trying to think of a better approach, but just thinking out loud here, maybe you shouldn't be able to provide oldProgram if you're calling snapshot.createProgram. If you're passing in an oldProgram, I'd expect us to use the old program's snapshot as base. As said above, I think when I did createProgram, the idea was for users to not have to worry about snapshots, but maybe that should change in the name of unifying the API.
There was a problem hiding this comment.
Hm. So. I think you still need to provide an oldProgram explicitly because you need it to find the old snapshot the old program was based on to lookup the old source files in. Otherwise you only have the new snapshot you want to build the new program from and have to assume everything is invalidated. Like, in the flow where you do
const p1 = snapshot.createProgram(...)
const s2 = snapshot.update(...)
const p2 = s2.createProgram(..., p1, ...)
the input snapshot to createProgram is not the snapshot p1 made - it's the newer more-derived one made by the update call (which is what's updating the filesystem contents in the new program). You could argue an API like
const p1 = api.createProgram(...)
const p2 = p1.snapshot.updateProgram(p1, p1.snapshot.update(...), ...)
could be nicer for program-centric API usage? But that's more program-oriented than snapshot-oriented, and afaik we're trying to be more snapshot-forward? We could always have both, I guess, the methods are identical so one will just offload to the other.
There was a problem hiding this comment.
I'm also not sure how snapshot-centric we want to be with createProgram, so I don't know if we want to have both approaches or stick to just one. Andrew Branch (@andrewbranch) do you have opinions?
There was a problem hiding this comment.
we do assume the old project we're passed was created from the same snapshot we're cloning, and therefore they agree in their view of the state of the file system.
Isn't this, like, of limited use? I thought the point of supplying an oldProgram was to diff state between old and new and reuse whatever we can from the old one. If the FS is unchanged between old and new, literally the only possible difference is the specified compiler options?
There was a problem hiding this comment.
It isn't. You'll still get a base snapshot from which you're cloning, and file changes on top of that base snapshot, so I don't know what you mean by "the FS is unchanged". The FS isn't unchanged, the point is that the old program (and therefore its project) is associated with an old snapshot, on top of which the FS changes will be processed. What I'm saying is we need to make sure old project and old snapshot agree.
There was a problem hiding this comment.
Isn't this, like, of limited use? I thought the point of supplying an oldProgram was to diff state between old and new and reuse whatever we can from the old one. If the FS is unchanged between old and new, literally the only possible difference is the specified compiler options?
We don't exactly diff state between two programs in Corsa. We use the FileChangeSummary to determine the scope of what could have changed, and if exactly one file in the old program was touched, we parse that one changed file and determine if the nature of the change means we can reuse the entirety of oldProgram save for that one file. If more than one file is touched, or the change could affect the module resolution graph, we rebuild the program from scratch, but notably we may have cached disk files from the base snapshot and cached SourceFiles from the session-based parse cache. (api.createProgram(..., oldProgram) doesn't mean you can't incorporate file system changes; there's an additional parameter that takes a file change summary; those files will be refreshed via the host file system.)
It feels like there are competing ideas for how to update state over time in the API that need to be reconciled. I'm brainstorming ways we can keep the central features of this PR while pulling back the degree to which we expose our internal state model, for now, to make sure we ship the right thing.
There was a problem hiding this comment.
I can drop the createProgram API for now - it's not terribly important since, like, directly creating programs (instead of snapshots) is kinda legacy anyway.
I see it as a layering thing - you can use the callbacks to supply the "host" fallback behavior that backs the in-memory/cache VFSes. You can load everything in up-front, but it's a lot easier to, say, just mount |
| return newSnapshotFileSystemWorker(params, base, currentDirectory, params.Kind == SnapshotFileSystemKindCache) | ||
| } | ||
|
|
||
| func newSnapshotFileSystemWorker(params *SnapshotFileSystem, base vfs.FS, currentDirectory string, layered bool) (vfs.FS, error) { |
There was a problem hiding this comment.
I think the layering needs to do some kind of eager compaction of intermediate cached FS layers to avoid potentially carrying a lot of wasted unreachable contents. If I made an IDE-like experience with repeated snapshot.update calls each time the user types a character in a file, each new snapshot's file system carries the full history of that file's contents.
There was a problem hiding this comment.
Hm, I wanna do this in a way that's forward-compatible with a world that allows multiple concurrent snapshots... so I guess I can do something like "when a snapshot is only owned by another (single) snapshot, it compacts into the parent and cleans itself up"? So if the client releases all refs to the old snapshots, the server's free to compress things.
There was a problem hiding this comment.
I don't think you need anything special here as long as you treat the base FS and all its entries as immutable. Apply the new layer on construction as a copy-on-write operation. You'll have a full clone of the underlying map, but any unaffected entries would share the same pointer. (This is how snapshotFS and a lot of things in the LSP layer work.) Then disposal is just garbage collection. IOW, you can always retain one baseFS as the underlying “total” FS (either a "memory" VFS or the host provided by the API server) and one overlay layer; any update that provides overlays eagerly creates a new overlay layer that merges the base snapshot's overlay layer with the newly provided ones, so it doesn't have to retain the base snapshot's overlay layer directly.
There was a problem hiding this comment.
I've moved and renamed some things for clarity, but I think I've found a good model where the core fs object is immutable while a mutable handle sits above it, and when that handle is released (because it's now defined to be 1:1 with a snapshot), it also takes the opportunity to compact and update any dependent FS that relies on it, so the FS object can be freed by the GC, too (along with any backing structures in that FS).
…s compaction on snapshot release, general cleanup
Just for fun, really. This is probably never going to be a bottleneck. It's just well-scoped.
Alongside
Snapshot.updatefor incrementally modifying a snapshot with a new cache overlay,createMemoryFileSystem,createMemoryFileSystemWithLib(for the common case where you need the default lib alongside your in-memory fs), andcreateCacheFileSystemhelper functions for creating the virtual filesystem parameter objects those API methods take."memory"VFSes are wholly in memory and do not fall back to host/session callback FS functionality."cache"VFSes will fallback to the host/callback fs for any cache misses. Both support symlinks both within the FS and out to the host FS, so even if your FS is nominally "all in memory", you can basically mount host folders as-needed."cache"VFSes can also specify "removed" paths that the cache layer blocks access to in the host. Use is like so:Note the
Object.entries- the helpers actually take[DocumentIdentifier, string][]for the core file contents set so you can reuseDocumentIdentifiers you get back from the API without needing to do explicit de-URI'ing yourself.Also note that despite the nice
.updateAPI, this doesn't actually relax the restriction that we only have one "real" snapshot alive at a time - and these vfs-backed snapshots count as "real" snapshots (unlike therunWithTemporaryFileUpdateoverlay), so you're very much so stuck operating serially (for now).cc Gabriela Araujo Britto (@gabritto) since we should probably consider how we want common mutations over the API to work holistically
Another aside: We might need to come up with a way to paginate the fs content in these update requests, since it doesn't take altogether too many >5MB source files to hit the ~500MB string length limit for the request message in js. Though in such cases, the workaround of using the chatty callback fs implementation still exists, so it's not incredibly pressing a need.
Fixes item 4A of #63875
Fixes #63855 probably?