feat(core): auto-resolve duplicate hostDirectives matches to avoid NG0309 (#57846)#64132
feat(core): auto-resolve duplicate hostDirectives matches to avoid NG0309 (#57846)#64132MillerSvt wants to merge 1 commit intoangular:mainfrom
hostDirectives matches to avoid NG0309 (#57846)#64132Conversation
…NG0309 (angular#57846) Previously, when the same `hostDirectives` configuration was applied multiple times in a component hierarchy, Angular produced an `NG0309: Directive has already been matched` error. This commit updates the resolution logic to automatically ignore subsequent matches of the same host directive. This change improves developer ergonomics by: - Eliminating the need for manual deduplication of hostDirectives. - Preventing runtime errors when multiple instances of the same directive are inherited or declared across different scopes. - Making host directive resolution behavior more predictable and consistent. PR Close angular#57846
| */ | ||
| function resolveHostDirectives(matches: DirectiveDef<unknown>[]): HostDirectiveResolution { | ||
| const allDirectiveDefs: DirectiveDef<unknown>[] = []; | ||
| const allDirectiveDefs = new Set<DirectiveDef<unknown>>(); |
There was a problem hiding this comment.
The reason why we haven't done this ourselves, but rather we throw the error, is that directives can influence the values during DI, as well as the order in which host bindings are applied. There isn't really a good way to know if we should pick the first match or one of the subsequent ones.
There was a problem hiding this comment.
Could you provide an example of code that will not work with that?
There was a problem hiding this comment.
If you had something like this, assuming that dirFoo is a directive that is applied twice as a host directive:
<div dirFoo dirBar dirBaz dirFoo></div>
The host bindings on the div will execute based on the matching order of the directives. This means that depending on if you decide to take the first instance of dirFoo or the second one, the result can be different. E.g. if both dirFoo and dirBar had a host binding of [class.active], taking the first instance of dirFoo will allow dirBar to override it, whereas if you take the second instance, dirFoo will override dirBar.
There was a problem hiding this comment.
<div dirFoo dirFoo></div> still produces NG0309
hostDirectives: [DirFoo, DirFoo] as well
(look at the tests)
There was a problem hiding this comment.
I was using the HTML to illustrate the execution order. This situation can happen if the host directives are applied through a selector-matched directive.
There was a problem hiding this comment.
We were discussing this within the team, but it wasn't actually clear in which specific cases it comes up. Can you point to some examples where this is a problem?
Whenever you actually use Directive Composition API for Composition -- you run into this all the time. I have a case in the issue linked to this PR, other examples:
- I have
Iconsdirective for displaying icons that is used across all other directives, such as buttons, chips, badges, textfields. Some of those directives can be combined on one element to be sum of both things, but I cannot do that because I get two instances of icon directive. - I have
Appearancedirective for displaying interactive state. It is used everywhere as well. Again that is preventing composition of two directives that on their own both have this already applied. - I have a directive that disables CSS transition initially so that there is no initial transition until it stabilizes (for various reasons CSS reflow can be triggered between addition to the DOM and applying host binding for current state which causes CSS transition) - this prevents me from combining entities that each need this safeguard on their own.
- I use directives rather than components everywhere because I need composition and I can have as many directives as I want but only one component per element. But directive infamously cannot have styles. I solved that by using dynamic components to store styles and wanted to use host directive for that, see this comment, but I can't due to double match issue.
These are just global encounters off the top of my head.
There was a problem hiding this comment.
Regarding having to alias everything.
- I have a directive that controls dropdowns, opening and closing them when user clicks or navigates away. Let's call it
[(dropdownOpen)]. This directive is used, for example, in textfields to open/close datalist for selects/date-pickers/autocomplete etc. Now imagine users also import this directive on their own because the need it on some other element like a dropdown menu button in the same view. Without aliasing they now get double match since the textfield already has it as a host directive.
I'm making these as concrete as I can to illustrate the issue, but really this is just a huge problem that hinders Directive Composition API very very much Basically every time you move some reusable logic into directive A and have directive B that needs it - you need to worry that there is no directive C that also needs it and that can be applied with B at the same time. And this happens a lot.
There was a problem hiding this comment.
@crisbeto can we prioritize this please? We need any solution as soon as possible. Currently we forced to create many class Directive1 extends Directive to avoid this error.
I'm not pushing exactly this solution, but we need any.
There was a problem hiding this comment.
We did decide to address this some time ago, but I haven't had time to sit down and figure out how we should handle it because of other priorities. I'll see if I can carve out some time...
|
There's a good chance Taiga UI is the biggest consumer of Directive Composition API out there. I can confidently say we really need this. Spartan is another library with over 100 |
Yeah, certainly something I have run into, and it would be nice to have a built in solution, but I understand the concern about DI - an opt in solution would be nice. |
|
I don't understand concern about DI. @crisbeto @ashley-hunter help me understand, please. Can you paint a scenario where it would matter which instance of a host directive is picked as the only one where it can potentially mess up providers that is not already a completely messed up case like relying on order of attributes on a host element to resolve a potentially conflicting providers. |
|
Directives are applied in the order in which they're matched in the template. Off the top of my head, there are at least three places in the framework that are affected by the order in which directives are applied: DI (e.g. when a directive has |
|
@waterplea How should we handle the situation when a host specifies its own directive, and users subsequently add the same directive to the same host within a template? |
Why would that matter? Same as if we met the same directive twice when resolving all of the directives applied to the host. It can be treated as host directive will all inputs/outputs exposed? At the end of the day what we need is:
|
|
Closing in favor of #67996. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Previously, when the same
hostDirectivesconfiguration was applied multiple times in a component hierarchy, Angular produced anNG0309: Directive has already been matchederror.Issue Number: #57846
What is the new behavior?
This commit updates the resolution logic to automatically ignore subsequent matches of the same host directive. If we define the same host directive within two another, and use them together, then first host directive instance is shared across two another host directives.
Exposed inputs/outputs maps of host directive, that applied multiple times, is merged. But if we expose same input multiple times with different aliases, NG0318 will raised (because of one input can have one source of truth).
If we define the same host directive multiple times in some directive/component, then NG0309 will raised.
Does this PR introduce a breaking change?
Other information