Conversation
…several templates. Replacing with signals
arcra
left a comment
There was a problem hiding this comment.
I think at least we should aim to remove the ! characters, possibly define default values.
…alues, and convert redundant selectors to computed signals
…ineLatest array form, camelCase naming Replaced deprecated Store.select(selector, props) with memoized selector factories across card renderer selectors
| ></tb-truncated-path> | ||
| <vis-linked-time-selection-warning | ||
| [isClipped]="linkedTimeSelection && linkedTimeSelection.clipped" | ||
| [isClipped]="(linkedTimeSelection && linkedTimeSelection.clipped) ?? false" |
There was a problem hiding this comment.
Can this be undefined? I think the boolean expression should evaluate to false if anything inside is undefined, so this looks a bit weird.
There was a problem hiding this comment.
Fixed. The wrapped calls in _reparent_children pass keyword arguments.
There was a problem hiding this comment.
@Jmats17 I think this landed on the wrong PR 🙂
There was a problem hiding this comment.
Yes @arcra, linkedTimeSelection itself can be null/undefined. Simplified to !!linkedTimeSelection?.clipped in all three
| ></tb-truncated-path> | ||
| <vis-linked-time-selection-warning | ||
| [isClipped]="linkedTimeSelection && linkedTimeSelection.clipped" | ||
| [isClipped]="(linkedTimeSelection && linkedTimeSelection.clipped) ?? false" |
There was a problem hiding this comment.
Same here and other places where the same expression exists.
| [yScaleType]="yScaleType ?? ScaleType.LINEAR" | ||
| [customXFormatter]="getCustomXFormatter()" | ||
| [tooltipTemplate]="tooltipTemplate" | ||
| [tooltipTemplate]="tooltipTemplate ?? undefined" |
There was a problem hiding this comment.
Is this ?? undefined doing anything?
Wouldn't the value still be undefined? This seems weird to me.
There was a problem hiding this comment.
It was mapping null to undefined, but null never actually reaches it. Removed null from the input type and dropped the ?? undefined
| export class VisLinkedTimeSelectionWarningComponent { | ||
| @Input() isClipped?: boolean = false; | ||
| @Input() isClosestStepHighlighted?: boolean = false; | ||
| @Input() isClipped: boolean | undefined = false; |
There was a problem hiding this comment.
Are the union types with null | undefined necessary here if we set a default value?
There was a problem hiding this comment.
Not anymore, with the template fix above isClipped can be plain boolean.
|
|
||
| @Input() | ||
| lastUpdated?: number; | ||
| lastUpdated: number | null | undefined; |
There was a problem hiding this comment.
Is it useful to declare it with null | undefined? Can it ever be null?
There was a problem hiding this comment.
null is valid before the first successful load, but undefined isn’t needed here. I’ll remove it.
| <ng-template #filterModalTemplate> | ||
| <tb-data-table-filter | ||
| [filter]="getCurrentColumnFilter()" | ||
| [filter]="getCurrentColumnFilter()!" |
There was a problem hiding this comment.
Yes, it's intentional. With strictNullInputTypes on, getCurrentColumnFilter() returns Filter | undefined but the filter input doesn't accept undefined.
Will replace it with with an @if (getCurrentColumnFilter(); as filter) guard so there's no assertion.
| ) { | ||
| return Object.entries(defaultFlags) | ||
| .filter(([flagName]) => { | ||
| if (!showFlagsFilter) { |
There was a problem hiding this comment.
nit: this could just be
return !showFlagsFilter || flagName.toLowerCase().includes(showFlagsFilter);
There was a problem hiding this comment.
Thank you, good suggestion.
Motivation for features / changes
Enable
strictNullInputTypesto strengthen Angular template type safety, catch nullable input bindings at build time, and prevent UI errors caused by loading or unavailable data. The change also modernizes synchronous NgRx selector bindings with Angular signals and reduces redundant reactive subscriptions.Technical description of changes
strictNullInputTypes: truein tsconfig.json.selectSignalortoSignal({requireSync: true}).