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
fix(core): host directive validation not picking up duplicate directives on component node #52073
Conversation
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 nit otherwise LGTM
if (seenDirectives.has(current)) { | ||
throw new RuntimeError( | ||
RuntimeErrorCode.DUPLICATE_DIRECTITVE, | ||
`Directive ${current.type.name} matches multiple times on the same element. ` + |
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.
nit: this needs the ngDevMode check to make the error message tree shakable.
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.
This function is prefixed with ngDevMode &&
at call sites, so the entire function should be tree-shaken-away in prod.
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.
@crisbeto LGTM 👍
(I think it'd be great to run TGP for this change)
163e561
to
bf2b102
Compare
Passing TGP. I also pushed a minor perf improvement. |
…ves on component node Fixes that, depending on the matching and import order, in some cases we weren't throwing the error saying that a directive matched multiple times on the same element. Fixes angular#52072.
bf2b102
to
5043325
Compare
This PR was merged into the repository by commit 7368b8a. |
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. |
…ves on component node (angular#52073) Fixes that, depending on the matching and import order, in some cases we weren't throwing the error saying that a directive matched multiple times on the same element. Fixes angular#52072. PR Close angular#52073
Fixes that, depending on the matching and import order, in some cases we weren't throwing the error saying that a directive matched multiple times on the same element.
Fixes #52072.