fix(compiler): fix directive registration order

fix angular#328

Author: vicb

modules/core/src/compiler/directive_metadata.js

@@ -1,4 +1,4 @@
-import {Type, FIELD} from 'facade/lang';
+import {Type} from 'facade/lang';
 import {Directive} from '../annotations/annotations'
 import {List} from 'facade/collection'
 import {ShadowDomStrategy} from './shadow_dom';

modules/core/src/compiler/pipeline/compile_element.js

@@ -1,6 +1,6 @@
 import {List, Map, ListWrapper, MapWrapper} from 'facade/collection';
 import {Element, DOM} from 'facade/dom';
-import {int, isBlank, isPresent} from 'facade/lang';
+import {int, isBlank, isPresent, Type} from 'facade/lang';
 import {DirectiveMetadata} from '../directive_metadata';
 import {Decorator} from '../../annotations/annotations';
 import {Component} from '../../annotations/annotations';
@@ -27,6 +27,7 @@ export class CompileElement {
   decoratorDirectives:List<DirectiveMetadata>;
   templateDirective:DirectiveMetadata;
   componentDirective:DirectiveMetadata;
+  _allDirectives:List<DirectiveMetadata>;
   isViewRoot:boolean;
   hasBindings:boolean;
   inheritedProtoView:ProtoView;
@@ -45,6 +46,7 @@ export class CompileElement {
     this.decoratorDirectives = null;
     this.templateDirective = null;
     this.componentDirective = null;
+    this._allDirectives = null;
     this.isViewRoot = false;
     this.hasBindings = false;
     // inherited down to children if they don't have
@@ -116,6 +118,7 @@ export class CompileElement {
 
   addDirective(directive:DirectiveMetadata) {
     var annotation = directive.annotation;
+    this._allDirectives = null;
     if (annotation instanceof Decorator) {
       if (isBlank(this.decoratorDirectives)) {
         this.decoratorDirectives = ListWrapper.create();
@@ -130,4 +133,23 @@ export class CompileElement {
       this.componentDirective = directive;
     }
   }
+
+  getAllDirectives(): List<DirectiveMetadata> {
+    if (this._allDirectives === null) {
+      // Collect all the directives
+      // When present the component directive must be first
+      var directives = ListWrapper.create();
+      if (isPresent(this.componentDirective)) {
+        ListWrapper.push(directives, this.componentDirective);
+      }
+      if (isPresent(this.templateDirective)) {
+        ListWrapper.push(directives, this.templateDirective);
+      }
+      if (isPresent(this.decoratorDirectives)) {
+        directives = ListWrapper.concat(directives, this.decoratorDirectives);
+      }
+      this._allDirectives = directives;
+    }
+    return this._allDirectives;
+  }
 }

modules/core/src/compiler/pipeline/element_binder_builder.js

@@ -58,7 +58,7 @@ export class ElementBinderBuilder extends CompileStep {
       if (isPresent(current.eventBindings)) {
         this._bindEvents(protoView, current);
       }
-      this._bindDirectiveProperties(this._collectDirectives(current), current);
+      this._bindDirectiveProperties(current.getAllDirectives(), current);
     } else if (isPresent(parent)) {
       elementBinder = parent.inheritedElementBinder;
     }
@@ -85,37 +85,21 @@ export class ElementBinderBuilder extends CompileStep {
     });
   }
 
-  _collectDirectives(compileElement) {
-    var directives;
-    if (isPresent(compileElement.decoratorDirectives)) {
-      directives = ListWrapper.clone(compileElement.decoratorDirectives);
-    } else {
-      directives = [];
-    }
-    if (isPresent(compileElement.templateDirective)) {
-      ListWrapper.push(directives, compileElement.templateDirective);
-    }
-    if (isPresent(compileElement.componentDirective)) {
-      ListWrapper.push(directives, compileElement.componentDirective);
-    }
-    return directives;
-  }
-
-  _bindDirectiveProperties(typesWithAnnotations, compileElement) {
+  _bindDirectiveProperties(directives: List<DirectiveMetadata>,
+                           compileElement: CompileElement) {
     var protoView = compileElement.inheritedProtoView;
-    var directiveIndex = 0;
-    ListWrapper.forEach(typesWithAnnotations, (typeWithAnnotation) => {
-      var annotation = typeWithAnnotation.annotation;
-      if (isBlank(annotation.bind)) {
-        return;
-      }
-      StringMapWrapper.forEach(annotation.bind, (dirProp, elProp) => {
+
+    for (var directiveIndex = 0; directiveIndex < directives.length; directiveIndex++) {
+      var directive = ListWrapper.get(directives, directiveIndex);
+      var annotation = directive.annotation;
+      if (isBlank(annotation.bind)) continue;
+      StringMapWrapper.forEach(annotation.bind, function (dirProp, elProp) {
         var expression = isPresent(compileElement.propertyBindings) ?
           MapWrapper.get(compileElement.propertyBindings, elProp) :
             null;
         if (isBlank(expression)) {
           throw new BaseException('No element binding found for property '+elProp
-            +' which is required by directive '+stringify(typeWithAnnotation.type));
+            +' which is required by directive '+stringify(directive.type));
         }
         var len = dirProp.length;
         var dirBindingName = dirProp;
@@ -129,7 +113,6 @@ export class ElementBinderBuilder extends CompileStep {
           isContentWatch
         );
       });
-      directiveIndex++;
-    });
+    }
   }
 }

modules/core/src/compiler/pipeline/proto_element_injector_builder.js

@@ -32,8 +32,7 @@ export class ProtoElementInjectorBuilder extends CompileStep {
   process(parent:CompileElement, current:CompileElement, control:CompileControl) {
     var distanceToParentInjector = this._getDistanceToParentInjector(parent, current);
     var parentProtoElementInjector = this._getParentProtoElementInjector(parent, current);
-    var injectorBindings = this._collectDirectiveBindings(current);
-
+    var injectorBindings = ListWrapper.map(current.getAllDirectives(), this._createBinding);
     // TODO: add lightDomServices as well,
     // but after the directives as we rely on that order
     // in the element_binder_builder.
@@ -65,22 +64,6 @@ export class ProtoElementInjectorBuilder extends CompileStep {
     return null;
   }
 
-  _collectDirectiveBindings(pipelineElement) {
-    var directiveTypes = [];
-    if (isPresent(pipelineElement.componentDirective)) {
-      ListWrapper.push(directiveTypes, this._createBinding(pipelineElement.componentDirective));
-    }
-    if (isPresent(pipelineElement.templateDirective)) {
-      ListWrapper.push(directiveTypes, this._createBinding(pipelineElement.templateDirective));
-    }
-    if (isPresent(pipelineElement.decoratorDirectives)) {
-      for (var i=0; i<pipelineElement.decoratorDirectives.length; i++) {
-        ListWrapper.push(directiveTypes, this._createBinding(pipelineElement.decoratorDirectives[i]));
-      }
-    }
-    return directiveTypes;
-  }
-
   _createBinding(d:DirectiveMetadata): DirectiveBinding {
     return DirectiveBinding.createFromType(d.type, d.annotation);
   }

modules/core/test/compiler/integration_spec.js

@@ -79,6 +79,22 @@ export function main() {
         });
       });
 
+      // GH issue 328 - https://github.com/angular/angular/issues/328
+      it('should support different directive types on a single node', (done) => {
+        compiler.compile(MyComp, el('<child-cmp my-dir [elprop]="ctxProp"></child-cmp>')).then((pv) => {
+          createView(pv);
+
+          ctx.ctxProp = 'Hello World!';
+          cd.detectChanges();
+
+          var elInj = view.elementInjectors[0];
+          expect(elInj.get(MyDir).dirProp).toEqual('Hello World!');
+          expect(elInj.get(ChildComp).dirProp).toEqual(null);
+
+          done();
+        });
+      });
+
       it('should support template directives via `<template>` elements.', (done) => {
         compiler.compile(MyComp, el('<div><template let-some-tmpl="greeting"><copy-me>{{greeting}}</copy-me></template></div>')).then((pv) => {
           createView(pv);
@@ -145,8 +161,10 @@ class MyComp {
 })
 class ChildComp {
   ctxProp:string;
+  dirProp:string;
   constructor(service: MyService) {
     this.ctxProp = service.greeting;
+    this.dirProp = null;
   }
 }
 

modules/core/test/compiler/pipeline/directive_parser_spec.js

@@ -62,6 +62,14 @@ export function main() {
         expect(results[0].componentDirective).toEqual(reader.read(SomeComponent));
       });
 
+      it('component directives must be first in collected directives', () => {
+        var results = createPipeline().process(el('<div some-comp some-decor></div>'));
+        var dirs = results[0].getAllDirectives();
+        expect(dirs.length).toEqual(2);
+        expect(dirs[0]).toEqual(reader.read(SomeComponent));
+        expect(dirs[1]).toEqual(reader.read(SomeDecorator));
+      });
+
       it('should detect them in property bindings', () => {
         var pipeline = createPipeline({propertyBindings: {
           'some-comp': 'someExpr'

modules/core/test/compiler/pipeline/element_binder_builder_spec.js

@@ -56,7 +56,8 @@ export function main() {
             if (isPresent(current.element.getAttribute('directives'))) {
               hasBinding = true;
               for (var i=0; i<directives.length; i++) {
-                current.addDirective(reflector.read(directives[i]));
+                var dirMetadata = reflector.read(directives[i]);
+                current.addDirective(dirMetadata);
               }
             }
             if (hasBinding) {
@@ -192,9 +193,9 @@ export function main() {
         'boundprop2': 'prop2',
         'boundprop3': 'prop3'
       });
-      var directives = [SomeDecoratorDirectiveWith2Bindings,
+      var directives = [SomeComponentDirectiveWithBinding,
                         SomeTemplateDirectiveWithBinding,
-                        SomeComponentDirectiveWithBinding];
+                        SomeDecoratorDirectiveWith2Bindings];
       var protoElementInjector = new ProtoElementInjector(null, 0, directives, true);
       var pipeline = createPipeline({
         propertyBindings: propertyBindings,

modules/core/test/compiler/pipeline/proto_element_injector_builder_spec.js

@@ -32,7 +32,8 @@ export function main() {
         }
         if (isPresent(current.element.getAttribute('directives'))) {
           for (var i=0; i<directives.length; i++) {
-            current.addDirective(reader.read(directives[i]));
+            var dirMetadata = reader.read(directives[i]);
+            current.addDirective(dirMetadata);
           }
         }
         current.inheritedProtoView = protoView;
@@ -133,9 +134,11 @@ export function main() {
 
 class TestableProtoElementInjectorBuilder extends ProtoElementInjectorBuilder {
   debugObjects:List;
+
   constructor() {
     this.debugObjects = [];
   }
+
   findArgsFor(protoElementInjector:ProtoElementInjector) {
     for (var i=0; i<this.debugObjects.length; i+=2) {
       if (this.debugObjects[i] === protoElementInjector) {
@@ -144,6 +147,7 @@ class TestableProtoElementInjectorBuilder extends ProtoElementInjectorBuilder {
     }
     return null;
   }
+
   internalCreateProtoElementInjector(parent, index, bindings, firstBindingIsComponent, distance) {
     var result = new ProtoElementInjector(parent, index, bindings, firstBindingIsComponent, distance);
     ListWrapper.push(this.debugObjects, result);