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
refactor: remove abstract directive selector workaround #17476
refactor: remove abstract directive selector workaround #17476
Conversation
bf0cf31
to
ec6253c
Compare
Removing this workaround unveiled two issues:
|
I think both of these things could be considered blockers for v9. So it might be worth prioritizing them in framework. cc. @jelbourn |
ec6253c
to
540be42
Compare
540be42
to
df88799
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Also I think that you can remove some extra logic from the coercion types lint rule which looks for do-no-use-
in the selector to determine whether a directive is abstract.
Since we updated the minimum required Angular version, and updated to Angular v9.0.0-0, we can remove the workaround for abstract directives. Previously we wanted to keep them with a selector regardless of the installed version, because we thought that we want to support ^8.0.0 of Angular.
df88799
to
db0d8b1
Compare
}) : | ||
null; | ||
const selectorText = | ||
selectorProp != null && ts.isStringLiteralLike(selectorProp.initializer) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need to look at the selector's text, just that the property is defined. I think the compiler will throw if the selector is invalid as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could still set the selector to undefined
, so I thought I'll cover that.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Since we updated the minimum required Angular version, and
updated to Angular
^9.0.0-0
, we can remove the workaroundfor abstract directives. Previously we wanted to keep them
with a selector regardless of the installed version, because
we thought that we want to support
^8.0.0
of Angular.