fix: improve null/undefined type handling for Angular wrapper #3302
Conversation
DavideMininni-Fincons
requested review from
kyubisation and
jeripeierSBB
as code owners
github-actions
bot
added
target: 2.x
pr: peer review required
| @@ -91,7 +91,7 @@ class SbbTagGroupElement extends SbbNamedSlotListMixin<SbbTagElement, typeof Lit | |||
| ? this.tags.filter((t) => t.checked).map((t) => t.value) | |||
| : (this.tags.find((t) => t.checked)?.value ?? null); | |||
| } | |||
| private _value: string | string[] | null = null; | |||
| private _value: string | (string | null)[] | null = null; | |||
There was a problem hiding this comment.
Can that be happen, an array consisting of null?
There was a problem hiding this comment.
value comes from the form-associated mixin, and it's nullable, so technically yes if all the tags have no value
| @@ -58,7 +58,7 @@ class SbbTagGroupElement extends SbbNamedSlotListMixin<SbbTagElement, typeof Lit | |||
| * If set multiple to true, the value is an array. | |||
| */ | |||
| @property() | |||
| public set value(value: string | string[] | null) { | |||
| public set value(value: string | (string | null)[] | null) { | |||
There was a problem hiding this comment.
Looks weird, do we expect that?
There was a problem hiding this comment.
I just kept the same type of the getter because it seems that the manifest takes the prop types from the getter (see the other comment on the tag-group about nullability)
| @@ -37,8 +37,8 @@ class SbbStepperElement extends SbbHydrationMixin(LitElement) { | |||
|
|
|||
| /** Overrides the behaviour of `orientation` property. */ | |||
| @property({ attribute: 'horizontal-from', reflect: true }) | |||
| public set horizontalFrom(value: SbbHorizontalFrom) { | |||
| this._horizontalFrom = breakpoints.includes(value) ? value : undefined; | |||
| public set horizontalFrom(value: SbbHorizontalFrom | undefined) { | |||
There was a problem hiding this comment.
Can't we do it with null (for all changes in this file)?
There was a problem hiding this comment.
change to null would require more changes (eg. default value for horizontal-from, change the SbbStepValidateEventDetails to match the nullable and so on)
| @@ -21,7 +21,7 @@ export declare class SbbNavigationActionCommonElementMixinType { | |||
| public accessor size: SbbNavigationActionSize; | |||
| public get marker(): SbbNavigationMarkerElement | null; | |||
| public get section(): SbbNavigationSectionElement | null; | |||
| public connectedSection: SbbNavigationSectionElement | null; | |||
| public connectedSection: SbbNavigationSectionElement | undefined; | |||
There was a problem hiding this comment.
What was the problem with null?
There was a problem hiding this comment.
On the manifest the type is SbbNavigationSectionElement | undefined because is taken from the mixin getter, while the generated component has null because uses the MixinType;
so when i create the setter in Angular I have a type mismatch
github-actions
bot
added
preview-available
pr: visual review required
diff-available
labels
Changes are related to Angular wrapper's classes generation.
sbb-calendar: normalized null / undefined types for getters and setters; needs some fix for thesbb-datepicker-togglesbb-navigation-action-common: updated mixin typesbb-slider: normalized typesbb-stepper: some minor fix to normalize null values, which are already considered in the existing logic but not in typingssbb-tag-group: normalized typesbb-time-input: add null as type as in the getter