Skip to content
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(expansion): enforce stricter types for inputs #20287

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

rafaelss95
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 12, 2020
@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch from e88e221 to eaaa368 Compare August 12, 2020 23:39
@rafaelss95 rafaelss95 changed the title refactor(cdk/accordion): enforce stricter types for inputs refactor(expansion): enforce stricter types for inputs Aug 12, 2020
@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch from baaaf36 to 8d75ba1 Compare August 12, 2020 23:52
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:

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

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

@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Aug 17, 2020
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM- we'll find out how breaking this is inside Google once we get closer to v11

@jelbourn jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: merge The PR is ready for merge by the caretaker labels Aug 17, 2020
@jelbourn
Copy link
Member

Caretaker note: Don't merge until v11 release train

@jelbourn
Copy link
Member

@rafaelss95 could you add a "BREAKING CHANGE" note to the commit message?

@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch 3 times, most recently from 89e3e9a to 5fe3def Compare August 19, 2020 02:09
@rafaelss95
Copy link
Contributor Author

@rafaelss95 could you add a "BREAKING CHANGE" note to the commit message?

Let me know if it's fine now.

@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch from 5fe3def to 7b3a797 Compare August 19, 2020 02:11
@jelbourn
Copy link
Member

@rafaelss95 just checked- the commit has to be a fix to show up in the changelog. You can also search (with /) through git log to see examples of other breaking changes blocks

@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch from 7b3a797 to 5d3367f Compare August 22, 2020 21:36
@rafaelss95 rafaelss95 changed the title refactor(expansion): enforce stricter types for inputs fix(expansion): enforce stricter types for inputs Aug 22, 2020
@rafaelss95
Copy link
Contributor Author

@jelbourn It should be ok now.

@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch 2 times, most recently from a45e3dd to b30b1eb Compare March 17, 2021 23:37
BREAKING CHANGE: The `disabled` and `expanded` properties in the `AccordionItem` are now strict to `boolean`.
@rafaelss95 rafaelss95 force-pushed the refactor/accordion-item-no-any branch from b30b1eb to e04e5d2 Compare March 18, 2021 00:29
@rafaelss95
Copy link
Contributor Author

@annieyw is there any chance to merge this before v12? :)

@annieyw annieyw merged commit 76a09d9 into angular:master Apr 6, 2021
@rafaelss95 rafaelss95 deleted the refactor/accordion-item-no-any branch April 6, 2021 09:31
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants