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

ivy: Boolean inputs cause compile errors #17495

Closed
asapach opened this issue Oct 24, 2019 · 12 comments
Closed

ivy: Boolean inputs cause compile errors #17495

asapach opened this issue Oct 24, 2019 · 12 comments
Assignees

Comments

@asapach
Copy link

asapach commented Oct 24, 2019

Reproduction

(StackBlitz doesn't seem to support ivy)

Steps to reproduce:

  1. npm install -g @angular/cli@next
  2. ng new app
  3. ng add @angular/material
  4. ng update @angular/material --next
  5. Add a basic button example with a boolean input:
<button mat-button disabled>Disabled</button>

Expected Behavior

The build should pass

Actual Behavior

ERROR in src/app/app.component.html:339:22 - error TS2322: Type 'string' is not assignable to type 'boolean'.

339   <button mat-button disabled>Disabled</button>
                         ~~~~~~~~

  src/app/app.component.ts:5:16
    5   templateUrl: './app.component.html',
                     ~~~~~~~~~~~~~~~~~~~~~~
    Error occurs in the template of component AppComponent.

This usage is prevalent in Material and CDK (e.g. cdkTrapFocus), so it's causing a lot of errors for consumers experimenting with ivy. Any input that relies on coerceBooleanProperty() is subject to this problem.

Environment

  • Angular: 9.0.0-next.13
  • CDK/Material: 9.0.0-next.0
  • Browser(s):
  • Operating System (e.g. Windows, macOS, Ubuntu):
@Airblader
Copy link
Contributor

Cf. angular/angular#33299

@manklu
Copy link

manklu commented Oct 24, 2019

As workaround you can use [disabled]="true"

@devversion
Copy link
Member

Yes. We are working on this. See #17483.

@asapach
Copy link
Author

asapach commented Oct 28, 2019

It looks like in one of the recent angular pre-releases they've disabled (or broke) the strict template checks by default (presumably here: angular/angular#33365), so this issue doesn't seem to manifest for now.

@manklu
Copy link

manklu commented Nov 1, 2019

disabled or disabled="true" now works with rc.0 and strict template checking.

There is still a problem with async pipe:

TS2326: Types of property 'opened' are incompatible.
  Type 'boolean | null' is not assignable to type 'string | boolean | undefined'.

@devversion
Copy link
Member

@manklu Thanks for reporting back. I'm actually trying to think about the async pipe case. I'm not sure if this is actually an issue from our side. The input property is typed to support string and boolean.

Mostly to support both scenarios:

<comp disabled></comp>
<comp [disabled]="boolean"></comp>

If the input binding is used with the async pipe, and no value is emitted yet, the async pipe will return null. This is correct, but I think it would make no sense to explicitly add null to the input type. If we'd go down that path, we'd need to make every input explicitly support null.

I think there is a flag called strictNullInputBindings which can help with that issue. Happy to discuss this more. I'm still not entirely sure what we can/should do about this.

@manklu
Copy link

manklu commented Nov 1, 2019

@devversion

I'm not sure if this is actually an issue from our side.

What is the alternative? One async pipe per data type which does the coercion? Magic from the compiler?

If we'd go down that path, we'd need to make every input explicitly support null.

At least most of the inputs already support null. The only problem is that the compiler doesn't know it.
If you have a good reason why null makes no sense for a particular input, I'm fine with it. But then it should also be enforced in non-IVY mode.

I think there is a flag called strictNullInputBindings which can help with that issue.

Hiding possible errors, just to be able to use the components with async pipe, can only be an interim solution. And if there is a company policy to enforce strict checks, it's not a solution at all.

@devversion
Copy link
Member

devversion commented Nov 1, 2019

What is the alternative? One async pipe per data type which does the coercion? Magic from the compiler?

The alternative is to just ensure that the value assigned to the input is always a boolean (if done through binding). i.e. by using !!(isDisabled$ | async)

If you have a good reason why null makes no sense for a particular input, I'm fine with it. But then > it should also be enforced in non-IVY mode.

Null makes sense from a convenience standpoint, but technically this is a boolean input expecting two values (i.e. true / false). We mostly support an empty string for the DX with empty attributes.

It's not enforced in view engine, because view engine's compiler is not smart enough about these things and doesn't even do proper input type checking. This is a new feature in the Angular Ivy compiler.

Hiding possible errors, just to be able to use the components with async pipe, can only be an interim solution. And if there is a company policy to enforce strict checks, it's not a solution at all.

It definitely is not a good idea to disable it, but with the current state, Ivy's strict template type checking is already hidden behind a flag called strictTemplates (disabled by default).

I've brought this up to the team and we are looking into it, but it's good to discuss this. I definitely see your point, but I'm also afraid that we do too much. The coercion is only intended to work for the two cases shown above. If someone assigns null to a boolean input, then this seems like a general/conceptual issue with how inputs work. This can also happen outside of Angular components. Most of the libraries just have an @Input() x: boolean where no coercion happens at all.

@manklu
Copy link

manklu commented Nov 3, 2019

@devversion Just a side note. The problem with asynchronous pipes doesn't just affect boolean values, it affects any input that does not support a null value.

@devversion
Copy link
Member

devversion commented Nov 3, 2019

Exactly. And that's why I think it's not an issue from the component side, but rather something general we need to think of. Sounds like it will not be a problem for angular/components only.

This is correct, but I think it would make no sense to explicitly add null to the input type. If we'd go down that path, we'd need to make every input explicitly support null.

@devversion devversion self-assigned this Nov 6, 2019
@jelbourn
Copy link
Member

This should be resolved as of a few releases ago

@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 Dec 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants