Skip to content
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

[heft-lint-plugin] fix: eslint doesn't cache output correctly #5107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aramissennyeydd
Copy link
Contributor

Summary

context: https://rushstack.zulipchat.com/#narrow/channel/262522-heft/topic/heft-lint-plugin.20blocking/near/499305386

It looks like the ESLint part of the heft-lint-plugin hasn't been caching correctly. This should significantly speed up rebuilds with the cache still set up.

Details

How it was tested

Tested against rush-lib, with a cache file the build time was ~2.2s and without the cache file it was ~7s. Before this change, it was a consistent 7s. Also verified that the cache file now has content.

"changes": [
{
"packageName": "@rushstack/heft-lint-plugin",
"comment": "Fix the cache only populating for incremental Typescript builds.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"comment": "Fix the cache only populating for incremental Typescript builds.",
"comment": "Fix an issue where the cache is only populated for incremental TypeScript builds.",

@@ -138,8 +138,15 @@ export abstract class LinterBase<TLintResult> {
continue;
}

// Compute the version from the source file content
const version: string = sourceFile.version || '';
// Typescript only computes the version during an incremental build.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Typescript only computes the version during an incremental build.
// TypeScript only computes the version during an incremental build.

Comment on lines +179 to +186
const transformedLintResults: TEslint.ESLint.LintResult[] = [];
for (const lintResult of lintResults) {
if (lintResult.messages.length > 0 || lintResult.warningCount > 0 || lintResult.errorCount > 0) {
transformedLintResults.push(lintResult);
}
}

return transformedLintResults;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const transformedLintResults: TEslint.ESLint.LintResult[] = [];
for (const lintResult of lintResults) {
if (lintResult.messages.length > 0 || lintResult.warningCount > 0 || lintResult.errorCount > 0) {
transformedLintResults.push(lintResult);
}
}
return transformedLintResults;
const trimmedLintResults: TEslint.ESLint.LintResult[] = [];
for (const lintResult of lintResults) {
if (lintResult.messages.length > 0 || lintResult.warningCount > 0 || lintResult.errorCount > 0) {
trimmedLintResults.push(lintResult);
}
}
return trimmedLintResults;

@@ -176,7 +176,14 @@ export class Eslint extends LinterBase<TEslint.ESLint.LintResult> {
return lintResult.fixableErrorCount + lintResult.fixableWarningCount > 0;
}));

return lintResults;
const transformedLintResults: TEslint.ESLint.LintResult[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unrelated to the caching issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs triage
Development

Successfully merging this pull request may close these issues.

3 participants