Skip to content

fix(expansion): enforce stricter types for inputs #20287

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 6, 2021
Merged

fix(expansion): enforce stricter types for inputs #20287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/cdk/accordion/accordion-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ export class CdkAccordionItem implements OnDestroy {

/** Whether the AccordionItem is expanded. */
@Input()
get expanded(): any { return this._expanded; }
set expanded(expanded: any) {
get expanded(): boolean { return this._expanded; }
set expanded(expanded: boolean) {
expanded = coerceBooleanProperty(expanded);

// Only emit events and update the internal value if the value changes.
Expand Down Expand Up @@ -90,9 +90,9 @@ export class CdkAccordionItem implements OnDestroy {

/** Whether the AccordionItem is disabled. */
@Input()
get disabled() { return this._disabled; }
set disabled(disabled: any) { this._disabled = coerceBooleanProperty(disabled); }
private _disabled: boolean = false;
get disabled(): boolean { return this._disabled; }
set disabled(disabled: boolean) { this._disabled = coerceBooleanProperty(disabled); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of using coercion here is that you can pass in anything. The problem is that TS doesn't support different types for the getter and the setter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I know. And I'm aware this is a breaking change and I wouldn't expect it to be merged until you reach v11 stages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were actually requiring these to be boolean, the coerceBooleanProperty call becomes redundant. I think that changing this defeats the purpose and we'd have to do it across the entire codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do actually use boolean for all boolean accessors, these here modified in this PR are outside the rule. It helps not only documentation, but the dev experience when accessing a getter, for example, you'd want a boolean, not any.

See these two examples for reference:

components/src/material/expansion/expansion-panel.ts

Lines 107 to 113 in cb8de61

@Input()
get hideToggle(): boolean {
return this._hideToggle || (this.accordion && this.accordion.hideToggle);
}
set hideToggle(value: boolean) {
this._hideToggle = coerceBooleanProperty(value);
}

components/src/cdk/drag-drop/directives/drop-list-group.ts

Lines 36 to 41 in cfe37d1

@Input('cdkDropListGroupDisabled')
get disabled(): boolean { return this._disabled; }
set disabled(value: boolean) {
this._disabled = coerceBooleanProperty(value);
}
private _disabled = false;

private _disabled = false;

/** Unregister function for _expansionDispatcher. */
private _removeUniqueSelectionListener: () => void = () => {};
Expand Down
2 changes: 1 addition & 1 deletion src/material/expansion/expansion-panel-header.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class MatExpansionPanelHeader implements AfterViewInit, OnDestroy, Focusa
* Whether the associated panel is disabled. Implemented as a part of `FocusableOption`.
* @docs-private
*/
get disabled() {
get disabled(): boolean {
return this.panel.disabled;
}

Expand Down
2 changes: 0 additions & 2 deletions src/material/expansion/expansion-panel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,6 @@ export class MatExpansionPanel extends CdkAccordionItem implements AfterContentI
}

static ngAcceptInputType_hideToggle: BooleanInput;
static ngAcceptInputType_expanded: BooleanInput;
static ngAcceptInputType_disabled: BooleanInput;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions tools/public_api_guard/cdk/accordion.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ export declare class CdkAccordionItem implements OnDestroy {
accordion: CdkAccordion;
closed: EventEmitter<void>;
destroyed: EventEmitter<void>;
get disabled(): any;
set disabled(disabled: any);
get expanded(): any;
set expanded(expanded: any);
get disabled(): boolean;
set disabled(disabled: boolean);
get expanded(): boolean;
set expanded(expanded: boolean);
expandedChange: EventEmitter<boolean>;
readonly id: string;
opened: EventEmitter<void>;
Expand Down
4 changes: 1 addition & 3 deletions tools/public_api_guard/material/expansion.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ export declare class MatExpansionPanel extends CdkAccordionItem implements After
ngOnDestroy(): void;
open(): void;
toggle(): void;
static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_expanded: BooleanInput;
static ngAcceptInputType_hideToggle: BooleanInput;
static ɵcmp: i0.ɵɵComponentDefWithMeta<MatExpansionPanel, "mat-expansion-panel", ["matExpansionPanel"], { "disabled": "disabled"; "expanded": "expanded"; "hideToggle": "hideToggle"; "togglePosition": "togglePosition"; }, { "opened": "opened"; "closed": "closed"; "expandedChange": "expandedChange"; "afterExpand": "afterExpand"; "afterCollapse": "afterCollapse"; }, ["_lazyContent"], ["mat-expansion-panel-header", "*", "mat-action-row"]>;
static ɵfac: i0.ɵɵFactoryDef<MatExpansionPanel, [{ optional: true; skipSelf: true; }, null, null, null, null, { optional: true; }, { optional: true; }]>;
Expand Down Expand Up @@ -100,7 +98,7 @@ export declare class MatExpansionPanelDescription {
export declare class MatExpansionPanelHeader implements AfterViewInit, OnDestroy, FocusableOption {
_animationMode?: string | undefined;
collapsedHeight: string;
get disabled(): any;
get disabled(): boolean;
expandedHeight: string;
panel: MatExpansionPanel;
constructor(panel: MatExpansionPanel, _element: ElementRef, _focusMonitor: FocusMonitor, _changeDetectorRef: ChangeDetectorRef, defaultOptions?: MatExpansionPanelDefaultOptions, _animationMode?: string | undefined);
Expand Down