Repository navigation
Add snapshot.rebase(newBaseSnapshot, changes) - #64679
Wesley Wigham (weswigham) wants to merge 3 commits into
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
🟡 Changes recommended
Missed invalidations can retain stale target contents or removed files in rebased snapshots.
3 open findings
What changed in this PR
Adds Snapshot.rebase to reuse synthetic filesystem changes over another snapshot without resending file contents.
Changes:
- Adds server, protocol, async, sync, and generator support.
- Implements filesystem composition and change notifications.
- Tests rebasing, invalidation, disposal, and auto-import behavior.
| File | Description |
|---|---|
| tsc/internal/api/session.go | Handles rebase requests. |
| tsc/internal/api/session_requestfilesystem_test.go | Tests self-rebase incremental state. |
| tsc/internal/api/session_rebase_test.go | Tests composition and snapshot lifetimes. |
| tsc/internal/api/session_rebase_changes_test.go | Tests invalidation and update parity. |
| tsc/internal/api/session_completion_test.go | Tests rebase auto-import preparation. |
| tsc/internal/api/requestfilesystem/requestfilesystem.go | Composes rebased filesystems. |
| tsc/internal/api/requestfilesystem/rebase_test.go | Tests input immutability. |
| tsc/internal/api/requestfilesystem/pathtree.go | Preserves removed-path metadata. |
| tsc/internal/api/requestfilesystem/filechanges.go | Generates rebase change notifications. |
| tsc/internal/api/requestfilesystem/filechanges_test.go | Tests notifications and path casing. |
| tsc/internal/api/proto.go | Defines the rebase protocol. |
| packages/typescript/test/sync/api.test.ts | Adds generated sync coverage. |
| packages/typescript/test/sync/api-generators.test.ts | Tests generator parity. |
| packages/typescript/test/async/api.test.ts | Adds async rebase coverage. |
| packages/typescript/src/api/sync/api.ts | Adds generated sync and generator APIs. |
| packages/typescript/src/api/proto.generated.ts | Adds generated protocol types. |
| packages/typescript/src/api/async/api.ts | Exposes the async rebase API. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| updatedSnapshot := session.snapshots[updated.Snapshot].snapshot | ||
| updatedProject := updatedSnapshot.ProjectCollection.GetProject(project.ID("/a/tsconfig.json")) | ||
| assert.Assert(t, updatedProject.GetProgram() != baseProgram) | ||
| assert.Equal(t, updatedProject.ProgramUpdateKind, project.ProgramUpdateKindCloned) |
There was a problem hiding this comment.
Related to my comment on session.go: this only hits ProgramUpdateKindCloned here because it's a self-rebase (Snapshot: params.Snapshot, NewSnapshot: params.Snapshot). In TestRebaseSnapshot (rebasing a layered snapshot with extra files onto a new getCurrentLanguageServerSnapshot()), the project ends up with ProgramUpdateKindNewFiles.
| } | ||
| apiRequest.FileSystem = fileSystem | ||
| apiRequest.ReplaceFileSystem = changes.FileSystem != nil && changes.FileSystem.Kind == requestfilesystem.KindFull | ||
| snapshot, err := s.snapshotHost.CloneSnapshot(ctx, target.snapshot, fileChanges, apiRequest) |
There was a problem hiding this comment.
In our case source.fileSystem has hundreds of virtual .ngtypecheck.ts files that don't exist in target.snapshot. Since handleRebaseSnapshot clones from target.snapshot and marks all of source.fileSystem's files as Created relative to target, will every rebase onto a new snapshot invalidate target's tsconfig file list and force compiler.NewProgram instead of ReuseProgram (ProgramUpdateKindCloned) when a user edits a single .ts file?
There was a problem hiding this comment.
Yeah, changes are related to target and not source so... yes, they are always "new" files (relative to the actual underlying base FS snapshot). If you wanted otherwise, we'd have to actually pull reused program state (and ASTs) from multiple parent programs, which is... I don't wanna think about it until it's shown we really have to. I'm not really sure how much we'd save by supporting it in practice - I'd need benchmarks showing it's actually majorly useful.
Alternatively, you could provide the chatty underlying fs lookup overrides to the lsp setup and retain the generated files on the "underlying real fs" that way, but I think ensuring consistency between that and the actual-real-fs is a headache you can potentially avoid with the snapshot-layering strategy.
|
I think the naming here led me to believe this would encompass more than just file system state, but we really don't carry enough info in snapshots for that to be possible. Maybe this should just be a variant of const initial = api.getCurrentLanguageServiceSnapshot()
const updated = initial.update(/* layer on custom additions */)
// fetch diagnostics for updated or whatever else you want
// actual disk/editor updates LSP snapshot
// ... later ...
const newInitial = api.getCurrentLanguageServiceSnapshot()
const reappliedUpdate = newInitial.update({
fileSystem: createFileSystemWithBase(updated)
});
// work with reappliedUpdate without needing to recalculate synthetic FS state
|
Much like
Snapshot.update,Snapshot.rebaseallows creating a new snapshot based on an old one, exceptSnapshot.rebaseuses data from two old snapshots to make a new snapshot.Given
const rebased = old.rebase(new), we take the synthetic FS stored inoldand reapply it overnew's existing FS data, if any (akin to agitrebase) to produce a new snapshot. (Do note: afullFS override inoldwill still replace the FS innew- it's a full replacement! The only thing that'd change here is refreshing underlying real FS files!) Optionally, we also take normalupdatearguments, if there are additional settings on the resulting snapshot update you wish to alter -snapshot.rebase(snapshot, settings)is basically identical tosnapshot.update(settings).What does this let you do? Well, for our LSP API users,
allowing you to retain the same synthetic FS structure you already generated across multiple real-FS updates without a lot of repeated traffic over the API protocol.