fix(material/datepicker): Return union of view component types #23000
Conversation
google-cla
bot
added
the
cla: yes
src/material/datepicker/calendar.ts
Outdated
| private _getCurrentViewComponent(): MatMonthView<D> | MatYearView<D> | MatMultiYearView<D> { | ||
| // The return type must be a union to ensure that the Closure compiler does not optimize the | ||
| // calls to _init(). See https://github.com/angular/components/issues/22996. | ||
| switch (this.currentView) { |
There was a problem hiding this comment.
Is this switch required for the Closure compiler fix? The comment sounds like only the return type change would be necessary.
There was a problem hiding this comment.
Only the return type change is necessary.
However, switch makes it clear that we chose the current view component and does not rely on implicit falsyness of class fields that are non-nullable. The code had two ways of selecting the component (ifs and ||), I unified them.
The comment is to ensure that future refactorings do not revert to || and reduce the return type to only one type. IMO, it is an easy mistake to remove the return type of:
returnValue(): A | B {
return this.a || this.b;
}
There was a problem hiding this comment.
It might just be me (also since I wrote the code to begin with), but I think I prefer the || since it's more compact and it doesn't depend on matching class instances to strings.
There was a problem hiding this comment.
Ok, changed back to use || and extended the comment with little more context.
I assume the existing code that did the string to view mapping (L391-L397) does not need to be restored, so I keep them unified.
crisbeto
added
P3
|
@rslawik it looks like your latest commit fails our commit title lint check. It needs to be prefixed with |
TypeScript inferred the return type of `_getCurrentViewComponent` as the type of the month view only. Tsickle propagates that to the JavaScript compiler which optimizes the code as `monthView._init` was the only method that could be called. This causes runtime errors. Fixed angular#22996
|
Thank you very much for the help, @crisbeto! It passed all the checks. |
TypeScript inferred the return type of `_getCurrentViewComponent` as the type of the month view only. Tsickle propagates that to the JavaScript compiler which optimizes the code as `monthView._init` was the only method that could be called. This causes runtime errors. Fixed #22996 (cherry picked from commit fafb24d)
TypeScript inferred the return type of `_getCurrentViewComponent` as the type of the month view only. Tsickle propagates that to the JavaScript compiler which optimizes the code as `monthView._init` was the only method that could be called. This causes runtime errors. Fixed #22996 (cherry picked from commit fafb24d)
|
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. |
TypeScript inferred the return type of
_getCurrentViewComponentas the type of the month view only. Tsickle propagates that to the JavaScript compiler which optimizes the code asmonthView._initwas the only method that could be called. This causes runtime errors.Fixed #22996