Cache keys include the import closure - #57
Merged
Merged
Conversation
johanrd
added
enhancement
An entry's key now covers the content of every project file the file imports, transitively, plus the project's ambient `.d.ts` files and its lockfile — so editing an imported component invalidates its consumers. Same shape as tsc's incremental `referencedMap`, keyed on content rather than on the exported signature. `lib/deps.ts` finds specifiers by text scan and resolves them through relative paths, tsconfig `paths`/`baseUrl` (following `extends`), `.js` → `.ts`, `.gts`/`.gjs`-first extension probing and directory `index` files; packages are external. Resolution is memoised per process; file content per mtime and size. Applies to the Glint, transform and report caches. Fixture project and scenario tests in test/deps-fixtures and test/deps.test.ts. Cowritten by Claude
johanrd
force-pushed
the
deps-cache-key
branch
from
August 29, 2026 09:59
7928178 to
e2a2141
Compare
Cowritten by Claude
Contributor
🏎️ Benchmark Comparison
Full output |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It reimplements module resolution (tsconfig paths/extends, JSONC parsing, extension probing) whose subtle edge cases directly affect cache-staleness correctness and warrant human review.
Pull request overview
This PR closes the cross-file caching caveat from #55/#56: previously a cache entry (Glint extraction, transform output, and CLI report) was keyed only on the file's own content, so editing an imported component did not invalidate its consumers. A new lib/deps.ts module computes a dependencySha — the hashed content of every project file a file imports transitively, plus the project's ambient .d.ts files and lockfile — and this sha is folded into all three cache keys. Imports are discovered by text scan and resolved through relative paths, tsconfig paths/baseUrl (following extends, JSONC-tolerant), TypeScript's .js→.ts rewrite, .gts/.gjs-first extension probing, and directory index files; anything not resolving to a project file is treated as external and covered by the lockfile sha. The approach mirrors tsc's incremental referencedMap but is content-keyed, making it stricter (never looser) than tsc.
Changes:
- Add
lib/deps.ts: import scanning, memoized module resolution, dependency-closure walk, anddependencySha. - Thread a
filenameargument intotransformCacheKey/reportCacheKeyand includedependencyShain the GlintreadCache/writeCache, transform, and report keys. - Add a
test/deps-fixturesproject plustest/deps.test.ts(13 tests) and updatetest/cache.test.ts/README accordingly.
File summaries
| File | Description |
|---|---|
| lib/deps.ts | New module: import discovery, tsconfig paths resolution, dependency closure, and dependencySha. |
| lib/cache.ts | Adds dependencySha to CacheEntry and all three cache keys; adds filename params. |
| transform.ts | Passes filename to transformCacheKey. |
| run.ts | Passes file to reportCacheKey. |
| test/deps.test.ts | New tests covering import scanning, resolution, closure, invalidation, and cache-key integration. |
| test/deps-fixtures/* | Small fixture project (aliases, .js→.ts, dir index, cycle, ambient .d.ts, lockfile). |
| test/cache.test.ts | Updates existing key calls for the new filename parameter. |
| README.md | Documents that caches now key on the import closure, ambient files, and lockfile. |
Review details
Files not reviewed (1)
- test/deps-fixtures/pnpm-lock.yaml: Generated file
- Files reviewed: 19/22 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Cowritten by Claude
… guns Review fixes for the import closure: - The tsconfig reader handles strings that end in a backslash, tolerates malformed `paths` and unparsable files (warns once, keeps resolving), follows `extends` arrays with later entries winning, resolves `paths` against the merged `baseUrl` or the declaring config's directory, prefers the longest pattern prefix, and follows package `extends` through the `tsconfig` field. - Resolved files are taken by real path; only `node_modules` is external, so workspace sources reached through a symlink or `../` are tracked. - Source files with `declare module` / `declare global` count as project-wide inputs next to `.d.ts` files. - Memos re-validate: resolutions and probes on the mtime of the directories they touched, tsconfig paths on the chain's content. The CLI opts into a static view of the file system for its run (`assumeStaticFileSystem`), like a non-watch tsc. - The Glint cache reads and writes under one dependency sha, computed once, so a dependency edited during the analysis is not stored as reflected. Not computed when the cache is off. - Without Glint the closure is not part of the key, and not computed. - `sha256` lives in one place; the cache header names the dependency sha. Cowritten by Claude
…go preload A `.gts`/`.gjs`/`.ts` file is keyed on its own import closure and the content of the project-wide inputs (ambient declarations, augmentations, lockfile): an edit invalidates its downstream consumers. A `.hbs` template has no imports and reaches components only through the registry, so it is keyed on the inputs with everything they import: an edit to any file the registry reaches invalidates every template. The tsgo preload reads and writes the Glint cache under one dependency sha like the ts6 preload; the CLI assumes a static file system from the start, before any preload. Directories are re-validated once per `dependencySha` instead of once per closure walk. Cowritten by Claude
Merged
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It reworks cache-invalidation correctness with a large new resolver/closure module whose stale-entry edge cases warrant a final human review despite comprehensive tests.
Review details
Files not reviewed (1)
- test/deps-fixtures/pnpm-lock.yaml: Generated file
- Files reviewed: 22/25 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+311
to
+312
| function dependenciesForKey(filename: string, contents: string, tsconfigPath: string | null): string { | ||
| return process.env['HVE_GLINT'] === '0' ? 'no-glint' : dependencySha(filename, contents, tsconfigPath); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the cross-file caveat from #55 and #56. Before this change, a cache entry was keyed on the content of one file only. An edit to an imported component did not invalidate the consumers of that component. The consumers kept a stale entry until they changed or the plugin version changed.
Now every cache (Glint, transform, report) includes
dependencyShain its key. This sha covers the content of every project file that the file imports, transitively. It also covers the ambient.d.tsfiles of the project and the lockfile (lib/deps.ts).This is the shape of the
referencedMapin the incremental mode of tsc. The key uses content, not the exported signature. As a result it is stricter than tsc, never looser.The scanner finds imports in the text. It resolves them in this order:
pathsandbaseUrl(withextends, jsonc accepted).jsto.tsrewrite.gtsand.gjsfirstindexfilesA specifier that does not resolve to a project file is a package. The lockfile sha covers packages.
Tests.
test/deps-fixturesis a small project: alias imports,.jsto.ts, a directory index, a"*": ["./types/*"]mapping, a cycle, an unrelated file and an ambient.d.ts.test/deps.test.tscovers the scenarios that the incremental build tests of TypeScript exercise:13 tests. Both lanes are green (309 passed, 1 expected fail).
What invalidates what.
.gts,.gjsor.tsfile is keyed on its own content, its import closure, and the content of the project-wide inputs (ambient.d.tsfiles, files withdeclare module/declare global, the lockfile). An edit invalidates the file and its downstream consumers. An edit to a project-wide input invalidates everything..hbstemplate has no imports. Components reach it only through the registry. So it is keyed on the project-wide inputs with everything they import. An edit to any file that the registry reaches invalidates every template. This is the.hbstrade-off: correct first..gts: a type that reaches a file only through a registry (@service declare session) is covered by the content of the service file, not by what the service file imports. A change in a type that the service imports, and that the consumer does not import, is not seen.Cost, console app (354
.gtsfiles, no.hbs, one process per run):statcalls in total).--no-glint: unchanged. Without Glint nothing crosses file boundaries, so the closure is not part of the key and is not computed.equipment-list.gts, 7 direct importers): 146 downstream consumers are recomputed. This is the difference between content and signature: a template-only edit invalidates everything downstream. It is the price of not having signatures.declare module '@ember/service'registry augmentation: every file is recomputed.Memos. File records re-validate on mtime and size. Resolutions and probes re-validate on the mtime of every directory they touched, so a file created later is seen. The tsconfig paths re-validate on the content of the config chain. The list of project-wide input files is found once per process.
validate-gtscallsassumeStaticFileSystem()at start and trusts the memos for its run, like a non-watchtsc. A long-lived host keeps re-validating.Review. A deep review found 10 items. Commits 2 and 3 fix the tsconfig reader (backslash in strings, malformed
paths,extendsorder, inheritedbaseUrl, longest prefix, packagetsconfigfield), track workspace sources by real path, count module augmentations as project-wide inputs, re-validate the memos, and read and write the Glint cache under one dependency sha in both backends. The one item not taken: a compiler-derived dependency list. The tsgo sync API exposesgetSourceFileNamesandgetSourceFileonly, with no per-file module resolution, so the text scanner stays.Cowritten by Claude