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(compiler): detect pipes in ICUs in template binder #38810
Conversation
Recent work on compiler internals in angular#38539 led to an unexpected failure, where a pipe used exclusively inside of an ICU would no longer be emitted into the compilation output. This caused runtime errors due to missing pipes. The issue occurred because the change in angular#38539 would determine the set of used pipes up-front, independent from the template compilation using the `R3TargetBinder`. However, `R3TargetBinder` did not consider expressions within ICUs, so any pipe usages within those expressions would not be detected. This fix unblocks angular#38539 and also concerns upcoming linker work, given that prelink compilations would not go through full template compilation but only `R3TargetBinder`.
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.
Always a joy to see such a simple fix!
visitIcu(icu: Icu): void {} | ||
visitIcu(icu: Icu): void { | ||
Object.keys(icu.vars).forEach(key => icu.vars[key].visit(this)); | ||
Object.keys(icu.placeholders).forEach(key => icu.placeholders[key].visit(this)); |
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.
Any particular reason to not use Object.values
?
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.
Yes; Object.entries
Object.values
requires es2017.object
to be available as lib, which we currently don't have enabled:
angular/packages/tsconfig-build.json
Line 18 in 2200884
"lib": ["es2015", "dom"], |
Recent work on compiler internals in #38539 led to an unexpected failure, where a pipe used exclusively inside of an ICU would no longer be emitted into the compilation output. This caused runtime errors due to missing pipes. The issue occurred because the change in #38539 would determine the set of used pipes up-front, independent from the template compilation using the `R3TargetBinder`. However, `R3TargetBinder` did not consider expressions within ICUs, so any pipe usages within those expressions would not be detected. This fix unblocks #38539 and also concerns upcoming linker work, given that prelink compilations would not go through full template compilation but only `R3TargetBinder`. PR Close #38810
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. |
Recent work on compiler internals in #38539 led to an unexpected failure,
where a pipe used exclusively inside of an ICU would no longer be
emitted into the compilation output. This caused runtime errors due to
missing pipes.
The issue occurred because the change in #38539 would determine the set
of used pipes up-front, independent from the template compilation using
the
R3TargetBinder
. However,R3TargetBinder
did not considerexpressions within ICUs, so any pipe usages within those expressions
would not be detected. This fix unblocks #38539 and also concerns
upcoming linker work, given that prelink compilations would not go
through full template compilation but only
R3TargetBinder
.