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

build: add lint rule to enforce coercion static properties for setters #17536

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

crisbeto
Copy link
Member

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 crisbeto added P2 The issue is important to a large percentage of users, with a workaround pr: merge safe target: major This PR is targeted for the next major release labels Oct 29, 2019
@crisbeto crisbeto requested review from jelbourn and a team as code owners October 29, 2019 21:11
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 29, 2019
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 pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 31, 2019
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM in general. I already ran this rule against my PR (a few instances are intentionally ignored due to the lint rule not being 100% correct; which is totally fine). These are the points we should improve though:

  • Checking if the setter is already typed to work with coercion. i.e. either contains null or string in the type
  • We should not warn for abstract directives or vanilla classes if they use coercion

@devversion
Copy link
Member

@crisbeto We chatted about this on Slack. do you want to look into these things as part of this PR or in follow-ups? Feel free to add merge ready based on what you prefer.

@devversion devversion removed the action: merge The PR is ready for merge by the caretaker label Oct 31, 2019
@crisbeto
Copy link
Member Author

I'll make the changes in this PR.

@crisbeto
Copy link
Member Author

Added the changes to skip abstract directives and to ignore setters which are any or string.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Oct 31, 2019
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM. One minor comment

tools/tslint-rules/coercionTypesRule.ts Outdated Show resolved Hide resolved
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.
@mmalerba mmalerba merged commit 716b259 into angular:master Nov 4, 2019
@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 5, 2019
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 P2 The issue is important to a large percentage of users, with a workaround 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