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.

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

feat: expand input types with coercion to work with ngtsc input type checking #17528

Merged
merged 5 commits into from
Oct 31, 2019
Merged

feat: expand input types with coercion to work with ngtsc input type checking #17528

merged 5 commits into from
Oct 31, 2019

Conversation

devversion
Copy link
Member

Related to #17495

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 28, 2019
@devversion
Copy link
Member Author

devversion commented Oct 28, 2019 •
edited
Loading

Blocked on:

  • bazel: unable to enable ngtsc strict template type checking angular#33452. We can workaround this and patch @angular/bazel in the meanwhile
    Resolution: There will be no changes to @angular/bazel since we need to audit how type checking should be configurable in Bazel. We will work around this from our side.

  • Input coercion static members cannot be used with NGC and strictMetadataEmit enabled angular#33451. We could disable strictMetadataEmit for the legacy build. This opens up the other question: why is this not enabled within Bazel?
    Possible solutions: We could add @dynamic to every class with such coercion members, or we set the members to null! (downside of generating code), or just add an exception to NGC metadata collector.

  • NgModel has an input called disabled that conflicts with the coerced disabled input if used on the same element. e.g. <mat-slider disabled ngModel>
    Possible solutions: Updating NgModel to also support coercion of the disabled input. Though IMO this is more like a work around. The error is actually valid and it shows that two inputs conflict in type, and only a value intersecting both types can be assigned. This makes sense to me. Since for NgModel it can be common that someone relies on the empty disabled attribute, it could make sense to update the input type.. though it can be a general problem for other directives too I guess

    For now, I just disabled the type checking on attributes which apply to inputs. That's exactly the use-case on why this strictness flag exists. We can re-enable this once we figured out how what to do with NgModel.

@devversion devversion force-pushed the accept-input-type branch 5 times, most recently from c6c7a1e to c163301 Compare October 29, 2019 10:23
crisbeto added a commit to crisbeto/material2 that referenced this pull request Oct 29, 2019
@crisbeto
build: add lint rule to enforce coercion static properties for setters
24d0781 
Adds a custom tslint rule to enforce that properties which use coercion in a setter also declare a static property to indicate the accepted types to ngtsc. Also handles inherited setters and properties coming from an interface being implemented (necessary to support mixins).

Relates to angular#17528.
@crisbeto crisbeto mentioned this pull request Oct 29, 2019
Merged
@devversion devversion force-pushed the accept-input-type branch 2 times, most recently from 90c626d to 5e696c1 Compare October 30, 2019 09:15
@devversion devversion marked this pull request as ready for review October 30, 2019 15:45
@devversion devversion added the target: major This PR is targeted for the next major release label Oct 30, 2019
@devversion devversion added this to the 9.0.0 milestone Oct 30, 2019
jelbourn
jelbourn approved these changes Oct 30, 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

@jelbourn jelbourn added P0 Issue that causes an outage, breakage, or major function to be unusable, with no known workarounds pr: lgtm action: merge The PR is ready for merge by the caretaker P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P0 Issue that causes an outage, breakage, or major function to be unusable, with no known workarounds labels Oct 30, 2019
@andrewseguin
Copy link
Contributor

Needs rebase

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Oct 31, 2019
jelbourn and others added 5 commits October 31, 2019 19:23
@jelbourn @devversion
feat: expand input types with coercion to work with ngtsc input type …
fc51dcc 
…checking
@devversion
refactor: add workaround for strict metadata emit of coercion static …
35430ff 
…members

See: angular/angular#33451
@devversion
build: patch ng_module bazel rule to enable strict template type chec…
2ab27c6 
…king

Workaround for: angular/angular#33452
@devversion
refactor: disable strict metadata emit for legacy build output
920102a 
Workaround for angular/angular#33451. This makes
sense at the current time since the Bazel output does *never* work with
strict metadata emit... and considering that with Ivy there is no metadata
anyway, it could be disabled in the meanwhile.
@devversion
build: disable strict attribute type checking
d9d5683 
We are temporarily disabling strict attribute type checking
since we have a few templates that set a coerced input while
the `NgModel` directive is applied. In those cases, the empty string
for the `disabled` input, is not assignable to the `NgModel#disabled` input,
since it does not do coercion. We need to figure out what to do in those
scenarios.
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Oct 31, 2019
@devversion
Copy link
Member Author

devversion commented Oct 31, 2019 •
edited
Loading

@andrewseguin Rebased. I had to fix a couple of newly introduced coercion inputs, so I think as soon as this is green, it would be good to merge it.

crisbeto added a commit to crisbeto/material2 that referenced this pull request Oct 31, 2019
@crisbeto
build: add lint rule to enforce coercion static properties for setters
1e7ef91 
Adds a custom tslint rule to enforce that properties which use coercion in a setter also declare a static property to indicate the accepted types to ngtsc. Also handles inherited setters and properties coming from an interface being implemented (necessary to support mixins).

Relates to angular#17528.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Oct 31, 2019
@crisbeto
build: add lint rule to enforce coercion static properties for setters
f7cb86f 
Adds a custom tslint rule to enforce that properties which use coercion in a setter also declare a static property to indicate the accepted types to ngtsc. Also handles inherited setters and properties coming from an interface being implemented (necessary to support mixins).

Relates to angular#17528.
@andrewseguin andrewseguin merged commit 8da64f4 into angular:master Oct 31, 2019
@manklu
Copy link

manklu commented Nov 1, 2019

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'.

mmalerba pushed a commit that referenced this pull request Nov 4, 2019
@crisbeto @mmalerba
build: add lint rule to enforce coercion static properties for setters (
716b259 
#17536)

Adds a custom tslint rule to enforce that properties which use coercion in a setter also declare a static property to indicate the accepted types to ngtsc. Also handles inherited setters and properties coming from an interface being implemented (necessary to support mixins).

Relates to #17528.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Nov 4, 2019
@crisbeto
build: enable coercion-types rule and fix leftover cases
f41cdfd 
Follow-up from angular#17528 that:
* Turns on the `coercion-types` rule.
* Fixes the rule not detecting abstract directives properly.
* Fixes the rule incorrectly flagging coercion functions used inside of callbacks which are inside of setters.
* Either fixes or works around a final set of failures.
@crisbeto crisbeto mentioned this pull request Nov 4, 2019
Merged
crisbeto added a commit to crisbeto/material2 that referenced this pull request Nov 4, 2019
@crisbeto
build: enable coercion-types rule and fix leftover cases
1ce6b3b 
Follow-up from angular#17528 that:
* Turns on the `coercion-types` rule.
* Fixes the rule not detecting abstract directives properly.
* Fixes the rule incorrectly flagging coercion functions used inside of callbacks which are inside of setters.
* Either fixes or works around a final set of failures.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Nov 4, 2019
@crisbeto
build: enable coercion-types rule and fix leftover cases
a6c6f5a 
Follow-up from angular#17528 that:
* Turns on the `coercion-types` rule.
* Fixes the rule not detecting abstract directives properly.
* Fixes the rule incorrectly flagging coercion functions used inside of callbacks which are inside of setters.
* Either fixes or works around a final set of failures.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Nov 4, 2019
@crisbeto
build: enable coercion-types rule and fix leftover cases
ede078b 
Follow-up from angular#17528 that:
* Turns on the `coercion-types` rule.
* Fixes the rule not detecting abstract directives properly.
* Fixes the rule incorrectly flagging coercion functions used inside of callbacks which are inside of setters.
* Either fixes or works around a final set of failures.
mmalerba pushed a commit that referenced this pull request Nov 4, 2019
@crisbeto @mmalerba
build: enable coercion-types rule and fix leftover cases (#17609)
05e6d07 
Follow-up from #17528 that:
* Turns on the `coercion-types` rule.
* Fixes the rule not detecting abstract directives properly.
* Fixes the rule incorrectly flagging coercion functions used inside of callbacks which are inside of setters.
* Either fixes or works around a final set of failures.
@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 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@crisbeto crisbeto Awaiting requested review from crisbeto

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: major This PR is targeted for the next major release
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@devversion @andrewseguin @manklu @jelbourn @googlebot