fix: improve source files invalidation #2942

Carlosamouco · 2024-10-27T22:07:36Z

only invalidate the *.ts file of a component when updating its template or style file
fixes problem where the template errors would not be shown when rebuilding

I'm submitting a...

Bug Fix
Feature
Other (Refactoring, Added tests, Documentation, ...)

Checklist

Commit Messages follow the Conventional Commits pattern
- A feature commit message is prefixed "feat:"
- A bugfix commit message is prefixed "fix:"
Tests for the changes have been added

Description

If we change a template or a style file of a component, we should only invalidate the associated *.ts file of that component and not other files or entry points. This helps reducing the amount of entry point rebuilds.

This also intends to fix a bug were when rebuilding a component template, the compiler would stop reporting diagnostics errors.

#2936

Does this PR introduce a breaking change?

Yes
No

alan-agius4 · 2024-10-28T07:36:12Z

src/lib/ts/cache-compiler-host.ts

@@ -289,3 +288,15 @@ export function augmentProgramWithVersioning(program: ts.Program): void {
    return files;
  };
 }
+
+export function invalidateProgramFiles(program: ts.Program, files: Set<string>): void {


It's odd, since TypeScript should manage this automatically. I'm fairly sure we don’t handle this in the Angular CLI.

Could you provide a detailed explanation or reproduction steps to show why this is needed?

//cc @JoostK, as he has a deep understanding of TypeScript internals.

I'd have to check some things to determine what is the case here. Is there a test that would show the issue and how these changes address it?

I noticed that when you save a component file for a couple of times that as errors in a template, suddenly, the diagnostics stop being reported and the entry points gets built normally.

I think the problem is that we are relying on the affectedFiles to run the angular diagnostics, and when you re-save a file without changing its content, that file is not marked as affected anymore by the getSemanticDiagnosticsOfNextAffectedFile.

compile-source-files.ts

// Only request Angular template diagnostics for affected files to avoid // overhead of template diagnostics for unchanged files. if (affectedFiles.has(sourceFile)) { const angularDiagnostics = angularCompiler.getDiagnosticsForFile(...); ... }

I was making sure those files are re-versioned so they are picked up again by the builder but maybe I could change that condition in some way?

On a final note, in the angular compiler cli, the builder is not reused again when there are errors, I think.

I noticed that when you save a component file for a couple of times that as errors in a template, suddenly, the diagnostics stop being reported and the entry points gets built normally.

Oh that is expected and working as intended from TS point of view. we should be storing and reusing previous errors and failures instead of invalidating the file unless it has actually changed.

On a final note, in the angular compiler cli, the builder is not reused again when there are errors, I think.

The compiler CLI does not utilize TypeScript builders, and it just re-uses the old program.

Ok that makes sense, I will update my PR then. I am thinking in comparing the version of the previous affected files with the version of the matching current files, and if they are the same, I will just retrieve the old diagnostics.

I have updated the code. I hope I am not wasting your time. :(

JoostK · 2024-10-28T07:50:06Z

src/lib/ts/cache-compiler-host.ts

@@ -289,3 +288,15 @@ export function augmentProgramWithVersioning(program: ts.Program): void {
    return files;
  };
 }
+
+export function invalidateProgramFiles(program: ts.Program, files: Set<string>): void {


I'd have to check some things to determine what is the case here. Is there a test that would show the issue and how these changes address it?

JoostK · 2024-10-28T07:54:06Z

src/lib/ts/cache-compiler-host.ts

+
+    if (source) {
+      source.version = createHash('sha256')
+        .update(source.text + new Date().getTime())


I don't quite understand the intent here. Adding the current time as part of the digest would mean the source file's content is mostly irrelevant. In any case, it would be better to split the string concatenation into two independent update calls.

I'd have to check how the ts.SourceFile.version is treated internally for better insight into what this means exactly.

alan-agius4 · 2024-11-26T08:43:54Z

src/lib/ngc/compile-source-files.ts

@@ -185,7 +194,10 @@ export async function compileSourceFiles(
  }

  if (errorDiagnostics.length) {
+    entryPoint.cache.affectedFiles = new Set(affectedFiles);


In this case, we should only cache the Angular diagnostics results, we should avoid re-running the diagnostics for files that did not actually change.

See: https://github.com/angular/angular-cli/blob/11289c4982dfaada860a595fa386d6176f696322/packages/angular/build/src/tools/angular/compilation/aot-compilation.ts#L184

I think I managed to find a solution for this using the existing FileCache.

alan-agius4 · 2024-12-05T07:49:53Z

src/lib/ngc/compile-source-files.ts

+    if (
+      affectedFiles.has(sourceFile) ||
+      !sourceFileCache.get(sourceFile.fileName)?.angularDiagnostics ||
+      sourceFileCache.get(sourceFile.fileName).angularDiagnostics.length
+    ) {
+      const angularDiagnostics =
+        (!affectedFiles.has(sourceFile) && sourceFileCache.get(sourceFile.fileName)?.angularDiagnostics) ||
+        angularCompiler.getDiagnosticsForFile(
+          sourceFile,
+          affectedFiles.size === 1 ? /** OptimizeFor.SingleFile **/ 0 : /** OptimizeFor.WholeProgram */ 1,
+        );

      allDiagnostics.push(...angularDiagnostics);
      sourceFileCache.updateAngularDiagnostics(sourceFile, angularDiagnostics);


Consider the below, to improve readability

Suggested change

if (

affectedFiles.has(sourceFile) ||

!sourceFileCache.get(sourceFile.fileName)?.angularDiagnostics ||

sourceFileCache.get(sourceFile.fileName).angularDiagnostics.length

) {

const angularDiagnostics =

(!affectedFiles.has(sourceFile) && sourceFileCache.get(sourceFile.fileName)?.angularDiagnostics) ||

angularCompiler.getDiagnosticsForFile(

sourceFile,

affectedFiles.size === 1 ? /** OptimizeFor.SingleFile **/ 0 : /** OptimizeFor.WholeProgram */ 1,

);

allDiagnostics.push(...angularDiagnostics);

sourceFileCache.updateAngularDiagnostics(sourceFile, angularDiagnostics);

if (affectedFiles.has(sourceFile)) {

const angularDiagnostics = angularCompiler.getDiagnosticsForFile(

sourceFile,

affectedFiles.size === 1 ? /** OptimizeFor.SingleFile **/ 0 : /** OptimizeFor.WholeProgram */ 1,

);

allDiagnostics.push(...angularDiagnostics);

sourceFileCache.updateAngularDiagnostics(sourceFile, angularDiagnostics);

} else {

const cachedDiagnostics = sourceFileCache.getAngularDiagnostics(sourceFile);

if (cachedDiagnostics?.length) {

allDiagnostics.push(...cachedDiagnostics);

}

}

We still need to account for cases where the file in FileCache was deleted by the file watcher due to a change or indirect change (in case of an update to a component template file) and diagnostics were lost/need to be re-evaluated.

const cachedDiagnostics = sourceFileCache.getAngularDiagnostics(sourceFile); if (affectedFiles.has(sourceFile) || !cachedDiagnostics) { ... }

As I said before, sometimes the component file is not reported as affected after a few saves to its template file. I noticed also that after a few saves in a component *.ts file ,without making any change, the affected entry points *.ts files stop being reported as affected, causing the same issue. If I then change something, like an input, no errors are reported in the affected entry points. Could this be a bug in the compiler?

The easiest way to solve this, would be to create a seperate cache, dedicated for diagnostics.

ng-packagr/src/lib/ng-package/nodes.ts

Line 103 in 0941cbd

sourcesFileCache: new FileCache(),

Happy to that this over, if you'd like.

I'm a little lost, I don't quite understand how creating a separate cache would solve this. The main problem that I am facing is the fact that the files are not getting affected by the compiler when they contain errors. If you want to tackle this please go ahead. Sorry in advance.

Do you have a test case that I can test against?
FYI: #2960

I tested you PR but I am still able to reproduce the problem. I will try to create the test cases.

I have added a two tests that reproduce the issue.

The first one saves the same template file a few times, without making any change on it, expecting always an error but it ends up compiling successfully after a couple of saves.

In the second test that I added I tried to do multiple saves on a valid component file and then change it's input name expecting to get an error in a secondary entry point. but it fails without even doing multiple saves.

thanks for this.

My first approach on this issue was to force a new version of a source file for unchanged files. While there might be an issue with the angular compiler, would it be possible to do this has a workaround? I really don't know where the problem is or a better way to solve it.

* only invalidate the *.ts file of a component when updating its template or style file * fixes problem where the template errors would not be shown when rebuilding a entry point

alan-agius4 reviewed Oct 28, 2024

View reviewed changes

JoostK reviewed Oct 28, 2024

View reviewed changes

Carlosamouco force-pushed the fix-watch-invalidate-source-files branch 2 times, most recently from e37e575 to 55eedce Compare October 28, 2024 13:58

alan-agius4 reviewed Nov 26, 2024

View reviewed changes

Carlosamouco force-pushed the fix-watch-invalidate-source-files branch 5 times, most recently from 5cff925 to 259a077 Compare November 26, 2024 20:58

alan-agius4 reviewed Dec 5, 2024

View reviewed changes

Carlosamouco force-pushed the fix-watch-invalidate-source-files branch from 259a077 to 5b61b0a Compare December 5, 2024 10:29

fix: improve source files invalidation

ec84e61

* only invalidate the *.ts file of a component when updating its template or style file * fixes problem where the template errors would not be shown when rebuilding a entry point

Carlosamouco force-pushed the fix-watch-invalidate-source-files branch from 5b61b0a to ec84e61 Compare December 6, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve source files invalidation #2942

fix: improve source files invalidation #2942

Carlosamouco commented Oct 27, 2024 •

edited

Loading

alan-agius4 Oct 28, 2024

JoostK Oct 28, 2024

Carlosamouco Oct 28, 2024

alan-agius4 Oct 28, 2024

Carlosamouco Oct 28, 2024

Carlosamouco Oct 28, 2024 •

edited

Loading

JoostK Oct 28, 2024

JoostK Oct 28, 2024

alan-agius4 Nov 26, 2024

Carlosamouco Nov 26, 2024

alan-agius4 Dec 5, 2024

Carlosamouco Dec 5, 2024 •

edited

Loading

alan-agius4 Dec 5, 2024

Carlosamouco Dec 5, 2024 •

edited

Loading

alan-agius4 Dec 5, 2024 •

edited

Loading

Carlosamouco Dec 5, 2024 •

edited

Loading

Carlosamouco Dec 5, 2024 •

edited

Loading

alan-agius4 Dec 10, 2024

Carlosamouco Dec 27, 2024 •

edited

Loading

fix: improve source files invalidation #2942

Are you sure you want to change the base?

fix: improve source files invalidation #2942

Conversation

Carlosamouco commented Oct 27, 2024 • edited Loading

I'm submitting a...

Checklist

Description

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carlosamouco Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carlosamouco Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carlosamouco Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

alan-agius4 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Carlosamouco Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Carlosamouco Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carlosamouco Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Carlosamouco commented Oct 27, 2024 •

edited

Loading

Carlosamouco Oct 28, 2024 •

edited

Loading

Carlosamouco Dec 5, 2024 •

edited

Loading

Carlosamouco Dec 5, 2024 •

edited

Loading

alan-agius4 Dec 5, 2024 •

edited

Loading

Carlosamouco Dec 5, 2024 •

edited

Loading

Carlosamouco Dec 5, 2024 •

edited

Loading

Carlosamouco Dec 27, 2024 •

edited

Loading