perf(compiler-cli): skip analysis in incremental builds for files wit…

[JoostK]

…hout Angular behavior

In an incremental rebuild, the compiler attempts to reuse as much
analysis data from a prior compilation as possible to avoid doing the
analysis work again. For source files without Angular behavior however,
no analysis data would be recorded such that the source file had to be
reanalyzed each rebuild, even if it has not changed.

This commit avoids the analysis of such source files by registering
these files as not containing any Angular behavior; allowing subsequent
rebuilds to avoid the analysis work.

[petebacondarwin]

This seems like a good optimisation. Thanks @JoostK.

One thing that slightly worries me is the use of null and undefined to have context specific meanings.

I think we should discuss the idea, going forward, of using well defined singleton branded objects that are then self-documenting, rather than relying upon documented specialized use of null.

For example in this case instead of null we could have: const NO_ANGULAR_TRAITS = { _brand: 'source file with no Angular traits' };.

[JoostK]

The null/undefined distinction is indeed not the nicest. Maybe an enum with two variants would be nicer.

Though there is precedent for using the null/undefined pattern elsewhere in the compiler code:

angular/packages/compiler-cli/src/ngtsc/shims/src/adapter.ts

Lines 137 to 144 in a92a89b

	/**
	* Produce a shim `ts.SourceFile` if `fileName` refers to a shim file which should exist in the
	* program.
	*
	* If `fileName` does not refer to a potential shim file, `null` is returned. If a corresponding
	* base file could not be determined, `undefined` is returned instead.
	*/
	maybeGenerate(fileName: AbsoluteFsPath): ts.SourceFile|null|undefined {

[AndrewKushnir]

Presubmit.

[AndrewKushnir]

Merge assistance: NgBot doesn't change the status to fully "green" due to one more reviewer assigned, but since Pete reviewed the change it might be ok to proceed without extra reviews? // cc @alxhub

[alxhub]

Is there much of a difference between using null here to indicate "analyzed, but had no traits" and using an empty array instead? I really try to avoid using null and undefined in the same type to indicate two different states.

[JoostK]

I considered that but found the null|undefined more explicit; the handling code in TraitCompiler.analyze is now forced to special-case putting null into this.filesWithoutTraits, whereas an empty array would not have required a change based on the type-system (and missed the necessity to update this.filesWithoutTraits)

[JoostK]

I don't feel particularly strongly about this, though, so happy to switch to [] if you think that's desirable.

[JoostK]

Ok, updated. That actually allowed most of the earlier changes to be reverted.

[JoostK]

merge-assistance: presubmit succeeded on an earlier revision; this should be fine to merge.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}