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

petebacondarwin · 2020-09-01T15:52:49Z

If a type has been renamed when it was exported, we need to
reference the external public alias name rather than the internal
original name for the type. Otherwise we will try to import the
type by its internal name, which is not publicly accessible.

Fixes #38238

packages/compiler-cli/ngcc/src/host/esm2015_host.ts

packages/compiler-cli/ngcc/src/host/commonjs_host.ts

packages/compiler-cli/src/ngtsc/reflection/src/host.ts

packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts

petebacondarwin · 2020-09-03T20:25:41Z

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 😵

petebacondarwin · 2020-09-04T17:33:04Z

@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

packages/compiler-cli/ngcc/src/host/esm2015_host.ts

petebacondarwin · 2020-09-05T07:13:55Z

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

petebacondarwin · 2020-09-05T07:29:18Z

packages/compiler-cli/ngcc/test/host/util.ts

@@ -13,26 +13,36 @@ import {CtorParameter, TypeValueReferenceKind} from '../../../src/ngtsc/reflecti
 * names.
 */
 export function expectTypeValueReferencesForParameters(
-    parameters: CtorParameter[], expectedParams: (string|null)[], fromModule: string|null = null) {
+    parameters: CtorParameter[], expectedParams: (string|null)[],
+    fromModule: (string|null)[] = []) {


Most of the changes here are simply to improve the failure messages

If a type has been renamed when it was exported, we need to reference the external public alias name rather than the internal original name for the type. Otherwise we will try to import the type by its internal name, which is not publicly accessible. Fixes angular#38238

gkalpak

👍

JoostK

I like it :-)

If a type has been renamed when it was exported, we need to reference the external public alias name rather than the internal original name for the type. Otherwise we will try to import the type by its internal name, which is not publicly accessible. Fixes #38238 PR Close #38666

matheo · 2020-09-08T19:10:58Z

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

petebacondarwin · 2020-09-08T20:10:25Z

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

matheo · 2020-09-08T20:23:56Z

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

petebacondarwin · 2020-09-08T20:44:52Z

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

matheo · 2020-09-09T21:20:50Z

Thank you @petebacondarwin

petebacondarwin · 2020-09-10T08:57:21Z

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

matheo · 2020-09-10T15:35:56Z

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

In angular#38666 we changed how ngcc deals with type expressions, where it would now always emit the original type expression into the generated code as a "local" type value reference instead of synthesizing new imports using an "imported" type value reference. This was done as a fix to properly deal with renamed symbols, however it turns out that the compiler has special handling for certain imported symbols, e.g. `ChangeDetectorRef` from `@angular/core`. The "local" type value reference prevented this special logic from being hit, resulting in incorrect compilation of pipe factories. This commit fixes the issue by manually inspecting the import of the type expression, in order to return an "imported" type value reference. By manually inspecting the import we continue to handle renamed symbols. Fixes angular#38883

#38892) In #38666 we changed how ngcc deals with type expressions, where it would now always emit the original type expression into the generated code as a "local" type value reference instead of synthesizing new imports using an "imported" type value reference. This was done as a fix to properly deal with renamed symbols, however it turns out that the compiler has special handling for certain imported symbols, e.g. `ChangeDetectorRef` from `@angular/core`. The "local" type value reference prevented this special logic from being hit, resulting in incorrect compilation of pipe factories. This commit fixes the issue by manually inspecting the import of the type expression, in order to return an "imported" type value reference. By manually inspecting the import we continue to handle renamed symbols. Fixes #38883 PR Close #38892

angular-automatic-lock-bot · 2020-11-01T16:25:14Z

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

petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release comp: ngcc labels Sep 1, 2020

petebacondarwin requested a review from gkalpak September 1, 2020 15:52

ngbot bot modified the milestone: needsTriage Sep 1, 2020

googlebot added the cla: yes label Sep 1, 2020

pullapprove bot requested a review from AndrewKushnir September 1, 2020 15:53

petebacondarwin marked this pull request as draft September 1, 2020 16:20

petebacondarwin removed the request for review from AndrewKushnir September 1, 2020 16:20

petebacondarwin force-pushed the ngcc-issue-38238 branch 2 times, most recently from d2717e0 to 14c5e4e Compare September 1, 2020 18:48

petebacondarwin marked this pull request as ready for review September 2, 2020 15:24

pullapprove bot requested a review from AndrewKushnir September 2, 2020 15:24

AndrewKushnir requested review from JoostK and removed request for AndrewKushnir September 2, 2020 18:11

JoostK reviewed Sep 2, 2020

View reviewed changes

packages/compiler-cli/ngcc/src/host/esm2015_host.ts Outdated Show resolved Hide resolved

gkalpak requested changes Sep 3, 2020

View reviewed changes

packages/compiler-cli/ngcc/src/host/commonjs_host.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/reflection/src/host.ts Outdated Show resolved Hide resolved

packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts Outdated Show resolved Hide resolved

petebacondarwin force-pushed the ngcc-issue-38238 branch from 14c5e4e to b3eda98 Compare September 4, 2020 17:28

petebacondarwin marked this pull request as draft September 4, 2020 17:33

petebacondarwin added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 4, 2020

JoostK reviewed Sep 4, 2020

View reviewed changes

packages/compiler-cli/ngcc/src/host/esm2015_host.ts Outdated Show resolved Hide resolved

petebacondarwin force-pushed the ngcc-issue-38238 branch 3 times, most recently from 9a5b522 to 9b11513 Compare September 5, 2020 07:13

petebacondarwin marked this pull request as ready for review September 5, 2020 07:13

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Sep 5, 2020

petebacondarwin requested review from JoostK and gkalpak September 5, 2020 07:14

petebacondarwin force-pushed the ngcc-issue-38238 branch from 9b11513 to 8e19743 Compare September 5, 2020 07:16

petebacondarwin commented Sep 5, 2020

View reviewed changes

petebacondarwin force-pushed the ngcc-issue-38238 branch from 8e19743 to 9b03169 Compare September 5, 2020 08:33

gkalpak approved these changes Sep 6, 2020

View reviewed changes

JoostK approved these changes Sep 6, 2020

View reviewed changes

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 6, 2020

atscott closed this in 7869de6 Sep 8, 2020

JoostK mentioned this pull request Sep 17, 2020

ChangeDetectorRef in Pipe used in Input resolves to wrong Components Instance #38883

Closed

JoostK mentioned this pull request Sep 17, 2020

fix(ngcc): fix compilation of ChangeDetectorRef in pipe constructors #38892

Closed

petebacondarwin deleted the ngcc-issue-38238 branch October 1, 2020 12:51

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petebacondarwin commented Sep 1, 2020

petebacondarwin commented Sep 3, 2020 •

edited

petebacondarwin commented Sep 4, 2020

petebacondarwin commented Sep 5, 2020

petebacondarwin Sep 5, 2020

gkalpak left a comment

JoostK left a comment

matheo commented Sep 8, 2020

petebacondarwin commented Sep 8, 2020

matheo commented Sep 8, 2020

petebacondarwin commented Sep 8, 2020

matheo commented Sep 9, 2020

petebacondarwin commented Sep 10, 2020

matheo commented Sep 10, 2020

angular-automatic-lock-bot bot commented Nov 1, 2020

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

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

Conversation

petebacondarwin commented Sep 1, 2020

petebacondarwin commented Sep 3, 2020 • edited

petebacondarwin commented Sep 4, 2020

petebacondarwin commented Sep 5, 2020

petebacondarwin Sep 5, 2020

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

matheo commented Sep 8, 2020

petebacondarwin commented Sep 8, 2020

matheo commented Sep 8, 2020

petebacondarwin commented Sep 8, 2020

matheo commented Sep 9, 2020

petebacondarwin commented Sep 10, 2020

matheo commented Sep 10, 2020

angular-automatic-lock-bot bot commented Nov 1, 2020

petebacondarwin commented Sep 3, 2020 •

edited