Skip to content

build: enable strict attribute type checking #17977

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

Jump to bottom
Merged
merged 1 commit into from
Jan 16, 2020
Merged

build: enable strict attribute type checking #17977

merged 1 commit into from
Jan 16, 2020

Conversation

devversion
Copy link
Member

When we initially enabled strict template type checking in the
components repository, we temporarily disabled the strict attribute
type checking due to an issue with NgModel and the disabled input.

NgModel seems to behave as expected and the issue we are facing for
some elements where NgModel#disabled is incompatible with the
disabled input of Angular Material components (where string is allowed)
is correctly reported by ngtsc.

We can satisfy both input signatures and actually enable the flag to
catch attribute typing/coercion issues which are actually affecting
components we ship.

@devversion devversion requested review from crisbeto, mmalerba and a team as code owners December 16, 2019 15:59
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 16, 2019
crisbeto
crisbeto reviewed Dec 16, 2019
View reviewed changes
src/dev-app/mdc-slide-toggle/mdc-slide-toggle-demo.html Outdated
@@ -4,7 +4,7 @@
Default Slide Toggle
</mat-slide-toggle>

<mat-slide-toggle [(ngModel)]="firstToggle" disabled>
<mat-slide-toggle [(ngModel)]="firstToggle" [disabled]="true">
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the whole point of those coercion types to allow usages like this?

Copy link
Member Author

@devversion devversion Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but in some cases where multiple directives are used on the same element, the union input type is used. In that case, the union of NgModel#disabled and MatSlideToggle#disabled is just boolean.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Shouldn't we fix NgModel in that case?

Copy link
Member Author

@devversion devversion Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I brought this up in the past when I disabled this flag/when I tried early Ivy type checking, but I think we didn't decide on an action. My thinking is that this is not an issue of NgModel. NgModel does not do coercion and ngtsc correctly reports that an empty string is not assignable.

Though we could say that this is such a common case in NgModel and we should do it for convenience. Happy to send a PR if so. Ultimately though this feels like a general problem not specific to only NgModel. There might be more other inputs in framework, or in third party libraries that cause these problems. Seems like another good reason for having something like @BooleanInput in the future. cc. @jelbourn

Copy link
Member

Choose a reason for hiding this comment

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

I would love to change NgModel to follow our model for disabled, but it is a breaking change and I think it's too late to make this change in v9. We just have to deal with it for now and file an issue to make the fix for v10.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually just read about angular/angular#19929 and it looks like NgModel already does coercion. So it seems like it would be the right thing to add a static coercion member?

Copy link
Member

Choose a reason for hiding this comment

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

So it does: https://github.com/angular/angular/blob/ac34a1429be08e05b110b87f8224d0c25e61a0ef/packages/forms/src/directives/ng_model.ts#L331

We should add the acceptance type, then, and then remove the disable bits here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I created angular/angular#34438

Copy link
Member Author

@devversion devversion Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Blocked on angular/angular#34502 😞

@devversion devversion force-pushed the build/enable-strict-attribute-types-checking branch from d2b9dd6 to 53f7c6b Compare December 16, 2019 16:21
@devversion devversion requested a review from jelbourn as a code owner December 16, 2019 16:21
@devversion devversion force-pushed the build/enable-strict-attribute-types-checking branch from 53f7c6b to f6ae8f4 Compare December 16, 2019 16:37
@devversion devversion added pr: merge safe target: patch This PR is targeted for the next patch release labels Dec 16, 2019
jelbourn
jelbourn approved these changes Dec 16, 2019
View reviewed changes
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

src/dev-app/mdc-slide-toggle/mdc-slide-toggle-demo.html Outdated
@@ -4,7 +4,7 @@
Default Slide Toggle
</mat-slide-toggle>

<mat-slide-toggle [(ngModel)]="firstToggle" disabled>
<mat-slide-toggle [(ngModel)]="firstToggle" [disabled]="true">
Copy link
Member

Choose a reason for hiding this comment

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

I would love to change NgModel to follow our model for disabled, but it is a breaking change and I think it's too late to make this change in v9. We just have to deal with it for now and file an issue to make the fix for v10.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Dec 16, 2019
@devversion devversion force-pushed the build/enable-strict-attribute-types-checking branch from f6ae8f4 to 2c8531e Compare December 19, 2019 21:28
@devversion
build: enable strict attribute type checking
6a544e7 
When we initially enabled strict template type checking in the
components repository, we temporarily disabled the strict attribute
type checking due to an issue with `NgModel` and the disabled input.

This has been fixed in the recent rc v9.0.0-rc.7
@devversion devversion force-pushed the build/enable-strict-attribute-types-checking branch from 2c8531e to 6a544e7 Compare January 16, 2020 19:37
@devversion
Copy link
Member Author

This is finally green. @jelbourn we should get it in to ensure that we test our coerced inputs.

@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Jan 16, 2020
@jelbourn jelbourn merged commit f46d7b3 into angular:master Jan 16, 2020
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
@devversion @yifange
build: enable strict attribute type checking (angular#17977)
a426eab 
When we initially enabled strict template type checking in the
components repository, we temporarily disabled the strict attribute
type checking due to an issue with `NgModel` and the disabled input.

This has been fixed in the recent rc v9.0.0-rc.7
@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 Feb 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto crisbeto left review comments

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@devversion @jelbourn @crisbeto @googlebot