Decorator added directives sometimes fail to compile

[kschaefe]

If you have a decorator for a directive and add a new directive to the returned array, that directive is never cleaned as it would be if it were added via $compileProvider.directive. This can lead to unexpected results during compilation and violate the principle of least surprise. See cases below, each assuming that the new directive added by the decorator does not include priority (expecting it to be treated as 0).

1. During normal compilation (no max priority) in addDirective in compile.js, the following line works as expected because the missing priority is never evaluated:

if ((maxPriority === undefined || maxPriority > directive.priority) &&
                 directive.restrict.indexOf(location) != -1) {

1. During a compilation with a priority, such as ngIf, the same line fails because maxPriority is defined and

maxPriority > directive.priority === false //because directive.priority is undefined

Since it should be valid to add directives during decoration, those directives should either be automatically cleaned or the compiler should perform safer checks for directive properties that have default values.

[lgalfaso]

Would it be possible for you to create a plunker for this specific issue? The information here is good, but would like to be sure that the specific use case is handled.

[kschaefe]

Here's the plunker:
http://plnkr.co/edit/8Aj9XkErSJ6KhA2ypMEh

If you uncomment the 'priority' in the decorator, then all test output will be green. In the default state, all the output inside the ngIf, the second item, is red.

[Narretz]

Hi @kschaefe, sorry for the late response. I see the problem in your plknr - when you add the directive directly into the delegate, you skip the registerDirective function from $compile. I agree that the behavior change is confusing; however your way of adding the directive is also pretty obscure. What's you use case for doing it like this? If you want to add a directive with the same name, you can either add it to the module via directive or use the $compileProviderdirectly, see this example: http://plnkr.co/edit/QNbEvdV7JpOA84SPUWcS?p=preview
It also shows that adding a directive to the $delegate skips parsing of the bindings completely - the definition would throw if added normally. So it's not only the priority that is affected by this.

[kschaefe]

I agree, @Narretz, that this is an atypical case, but it is certainly not out of the realm of possibility.

Our use case stems from the need to have a common set of attributes applied to directives. There are a couple of issues related to that, such as: not wanting place the same attributes in every template, the need to be able to quickly change the definition of a common attribute. Basically, concerns that you might have if you were trying to address the issue in an aspect-oriented way.

Since angular does not support attribute-sets out of the box, there are several approaches online that recommend ways to tackle this problem. The most common is to have developer annotate the template with an attribute that is itself a directive that explodes into the set of attributes that you need (only works for modifying templates and requires it to be present on the original template). This approach requires the use of terminal directives and $compile restarts if the attribute set contains attribute directives.

Personally, I hate that approach, but this is not a condemnation of that method. My choice was to create second directive that simply attaches the common attributes. My chief problem is that I do not want to create a new common-attribute directive for each directive manually. Therefore, I programmatically create one when the original directive is added via an overridden module.directive method.

At the current step it would be possible to simply use $compileProvider to pass the newly generated directive into angular if I knew all of the properties for the directive definition, but I don't know all of the directive's properties. A secondary reason for using the decorator over $compileProvider is the ability to execute checks prior to return the decorator. For instance, we cannot decorate a directive with common attributes if replace=true. Among the properties that we do not know are restrict, since we only want to match what the original directive did.

I can create another plnkr to help explain that use case, but I'd need a little time to divest our code of some identifying bits first. Let me know if that is necessary.

[Narretz]

I think your use case is unusual but valid, but I also think that when you are accessing / modifying the $delegate property, it's expected that you need to be extra careful because you have to make sure that the instance you provide matches what the consumer expects. This is no different with a directive, just a bit more complex.

Also, adding another set of validations after the service has been instantiated could add a significant overhead to the compile time, and this is not something we want for such a rare use case.
There's also the thing that for any delegate

But you are right that this is surprising and docs about decorators have actually been on my list for a while (#12163)