fix(ngcc): use aliased exported types correctly - #38666

Thoughts from @JoostK :

viaModule is about external modules, so relative imports won’t have that in which case we’d currently label it as LOCAL type reference, where it really should be IMPORTED
ngtsc typically uses Reference and a ReferenceEmitter to generate an expression that can be used in any context; the type-to-value conversion however does it more manually and assumes that the type reference that it creates is emitted in the same file as it was extracted from
but it will still generate its own import statement for imported symbols for IMPORTED type references; LOCAL however will be emitted as is

in ngcc we do currently use LOCAL even if a relative module import was used; which probably works out fine because it can simply emit the exact same expression in the same file. ngtsc can’t rely on that because it would break for symbols that are only used in type-positions, therefore the import is elided.
In ngcc’s case, I think that the right way to go about it is to use the code I proposed to figure out whether an import is used in the first place, and use that information to either return an IMPORTED type reference or a LOCAL one
so that we’re no longer relying on viaModule, as that’s only correct in few cases.

so the primary job of ngtsc’s typeToValue is to go from a ts.TypeNode to something that eventually becomes a ts.Expression. The TypeValueReference is a way to represent the various possibilities, where it differentiates between local and imported symbols to be able to properly emit import statements (as that is required to safely go from a ts.TypeNode to a ts.Expression).
In ngcc however, the situation is very simple: we already have typeExpression which is a ts.Expression, so we might not need the intermediate representation for an IMPORTED type-value-reference. From that perspective, it may actually be perfectly fine to always use that ts.Expression as a LOCAL type (as it already does if viaModule is null) and not bother with synthesizing a different ts.Expression based on derived information about the original ts.Expression 😵

@JoostK and @gkalpak I followed Joost's advice and began to unwind what I had written and use his idea. See the beginnings of it here

OK, I have gone with the simple approach suggested by @JoostK. It appears to work in a reproduction of the original issue for me locally. I have added tests for each of the ngcc ReflectionHosts.

PTAL

Will this be released for 9.x too? or just for 10.x?

^9.0.0 is now in LTS and so I don't expect this fix to land there.

but then how can it be LTS if it missed this? :(

@matheo - LTS versions only receive "critical fixes and security patches". I accept that it is somewhat subjective as to what a "critical fix" is. I will check with the team tomorrow.

Thank you @petebacondarwin

Hi @matheo - we discussed this last night and I am afraid that this PR does not constitute a critical fix for LTS.

I have put together a PR to change the docs to clarify what fixes will land on LTS versions: #38788

Got it, ok, we'll start moving to Angular 10 then :)
Thanks for this fix!

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.}