-
Notifications
You must be signed in to change notification settings - Fork 653
[heft-lint-plugin] Stabilize linter cache filename using tsconfig path hash #5437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: dmichon-msft <[email protected]>
dmichon-msft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get the config file path from the program's getCompilerOptions().configFilePath (may require null checking). This will greatly simplify the code change.
| accessor.onChangedFilesHook.tap(PLUGIN_NAME, (changedFilesHookOptions: IChangedFilesHookOptions) => { | ||
| // When using the TypeScript plugin, we need to determine the tsconfig path | ||
| // The default tsconfig path is used when not explicitly specified | ||
| const tsconfigPath: string = path.resolve(heftConfiguration.buildFolderPath, 'tsconfig.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is not documented on the public API typings, changedFilesHookOptions.program.getCompilerOptions().configFilePath is the resolved path to the tsconfig file used by the program, and simply needs to be converted to a relative path and be run through Path.convertToSlashes.
heft-typescript-plugin already uses this code path, so the custom extended type should already be defined in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit ba0baea. Now extracting configFilePath directly from program.getCompilerOptions().configFilePath.
| taskSession | ||
| ); | ||
| typescriptChangedFiles.push([tsProgram, new Set(tsProgram.getSourceFiles())]); | ||
| typescriptChangedFiles.push([tsProgram, new Set(tsProgram.getSourceFiles()), tsconfigPath]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the path is extractable from the program, you don't need to forward it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit ba0baea. Removed the separate forwarding of tsconfigFilePath.
| .digest('base64') | ||
| .replace(/\+/g, '-') | ||
| .replace(/\//g, '_') | ||
| .slice(0, 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .digest('base64') | |
| .replace(/\+/g, '-') | |
| .replace(/\//g, '_') | |
| .slice(0, 8); | |
| .digest('base64url'); |
No need to truncate since it is not in a length-limited location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit ba0baea. Changed to base64url encoding without truncation for filesHash.
| .digest('base64') | ||
| .replace(/\+/g, '-') | ||
| .replace(/\//g, '_') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .digest('base64') | |
| .replace(/\+/g, '-') | |
| .replace(/\//g, '_') | |
| .digest('base64url') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit ba0baea. Changed to base64url encoding for the cache filename hash suffix.
| const fileHash: Hash = createHash('md5'); | ||
| // Calculate the hash of the list of filenames for verification purposes | ||
| const filesHash: Hash = createHash('md5'); | ||
| for (const file of options.typeScriptFilenames) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy this array and then sort it to ensure a stable hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit ba0baea. Now sorting the file paths array before hashing to ensure stable hash values.
|
@copilot address feedback |
…se64url encoding, and sort files Co-authored-by: dmichon-msft <[email protected]>
Done in commit ba0baea. The implementation now uses program.getCompilerOptions().configFilePath as suggested, which greatly simplified the code. |
Summary
The linter cache filename used a hash of all input filenames, causing cache churn when files were added/removed. This stabilizes the cache filename by hashing the tsconfig path instead, while moving the files hash into the cache data for validation.
Fixes #5436
Details
Cache filename generation (
LinterBase.ts):program.getCompilerOptions().configFilePathILinterCacheData.filesHashfield for cache validationbase64urlencoding for hashes (filesHash without truncation, hashSuffix truncated to 8 chars for filename)cacheVersionorfilesHashchangesSimplified implementation (
LintPlugin.ts):Before:
After:
How it was tested
Impacted documentation
None - this is an internal implementation change that does not affect the public API or user-facing behavior.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.