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
fix(ngcc): fix compilation of ChangeDetectorRef
in pipe constructors
#38892
Conversation
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
@@ -1369,7 +1369,7 @@ runInEachFileSystem(() => { | |||
name: _('/main.js'), | |||
contents: ` | |||
(function (global, factory) { | |||
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib), require('./local')) : | |||
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib'), require('./local')) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was fun to track down 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥵
emitDecoratorMetadata: true, | ||
moduleResolution: ts.ModuleResolutionKind.NodeJs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need emitDecoratorMetadata
to emit the constructor params that ngcc can reflect upon, and moduleResolution
was required to actually resolve the @angular/core
imports. Without it, the constructor metadata would be emitted using a "safe" form, something like typeof ChangeDetectorRef != 'undefined' && ChangeDetectorRef
, which obviously wouldn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and super fast work @JoostK
#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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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 valuereference 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