refactor(core): throw more descriptive error message in case of invalid host element (angular#35916)

AndrewKushnir · AndrewKushnir · commit 0879d2e85d29 · 2020-06-26T11:10:14.000-07:00
This commit replaces an assert with more descriptive error message that is thrown in case `<ng-template>` or `<ng-container>` is used as host element for a Component. Resolves angular#35240. PR Close angular#35916
diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json
@@ -62,7 +62,7 @@
         "bundle": "TODO(i): we should define ngDevMode to false in Closure, but --define only works in the global scope.",
         "bundle": "TODO(i): (FW-2164) TS 3.9 new class shape seems to have broken Closure in big ways. The size went from 169991 to 252338",
         "bundle": "TODO(i): after removal of tsickle from ngc-wrapped / ng_package, we had to switch to SIMPLE optimizations which increased the size from 252338 to 1198917, see PR#37221 and PR#37317 for more info",
-        "bundle": 1209688
+        "bundle": 1210239
       }
     }
   }
diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts
@@ -225,7 +225,7 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
         createElementRef(viewEngine_ElementRef, tElementNode, rootLView), rootLView, tElementNode);
 
     // The host element of the internal root view is attached to the component's host view node.
-    ngDevMode && assertNodeOfPossibleTypes(rootTView.node, TNodeType.View);
+    ngDevMode && assertNodeOfPossibleTypes(rootTView.node, [TNodeType.View]);
     rootTView.node!.child = tElementNode;
 
     return componentRef;
diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts
@@ -267,7 +267,7 @@ export function diPublicInInjector(
 export function injectAttributeImpl(tNode: TNode, attrNameToInject: string): string|null {
   ngDevMode &&
       assertNodeOfPossibleTypes(
-          tNode, TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer);
+          tNode, [TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer]);
   ngDevMode && assertDefined(tNode, 'expecting tNode');
   if (attrNameToInject === 'class') {
     return tNode.classes;
diff --git a/packages/core/src/render3/instructions/di.ts b/packages/core/src/render3/instructions/di.ts
@@ -9,8 +9,7 @@ import {InjectFlags, InjectionToken, resolveForwardRef} from '../../di';
 import {ɵɵinject} from '../../di/injector_compatibility';
 import {Type} from '../../interface/type';
 import {getOrCreateInjectable, injectAttributeImpl} from '../di';
-import {TDirectiveHostNode, TNodeType} from '../interfaces/node';
-import {assertNodeOfPossibleTypes} from '../node_assert';
+import {TDirectiveHostNode} from '../interfaces/node';
 import {getLView, getPreviousOrParentTNode} from '../state';
 
 /**
diff --git a/packages/core/src/render3/instructions/listener.ts b/packages/core/src/render3/instructions/listener.ts
@@ -128,7 +128,7 @@ function listenerInternal(
 
   ngDevMode &&
       assertNodeOfPossibleTypes(
-          tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer);
+          tNode, [TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer]);
 
   let processOutputs = true;
 
diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts
@@ -15,6 +15,7 @@ import {assertDataInRange, assertDefined, assertDomNode, assertEqual, assertGrea
 import {createNamedArrayType} from '../../util/named_array_type';
 import {initNgDevMode} from '../../util/ng_dev_mode';
 import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect';
+import {stringify} from '../../util/stringify';
 import {assertFirstCreatePass, assertLContainer, assertLView} from '../assert';
 import {attachPatchData} from '../context_discovery';
 import {getFactoryDef} from '../definition';
@@ -272,7 +273,7 @@ export function assignTViewNodeToLView(
   let tNode = tView.node;
   if (tNode == null) {
     ngDevMode && tParentNode &&
-        assertNodeOfPossibleTypes(tParentNode, TNodeType.Element, TNodeType.Container);
+        assertNodeOfPossibleTypes(tParentNode, [TNodeType.Element, TNodeType.Container]);
     tView.node = tNode = createTNode(
                              tView,
                              tParentNode as TElementNode | TContainerNode | null,  //
@@ -1278,7 +1279,7 @@ function instantiateAllDirectives(
     const isComponent = isComponentDef(def);
 
     if (isComponent) {
-      ngDevMode && assertNodeOfPossibleTypes(tNode, TNodeType.Element);
+      ngDevMode && assertNodeOfPossibleTypes(tNode, [TNodeType.Element]);
       addComponentLogic(lView, tNode as TElementNode, def as ComponentDef<any>);
     }
 
@@ -1366,7 +1367,7 @@ function findDirectiveDefMatches(
   ngDevMode && assertFirstCreatePass(tView);
   ngDevMode &&
       assertNodeOfPossibleTypes(
-          tNode, TNodeType.Element, TNodeType.ElementContainer, TNodeType.Container);
+          tNode, [TNodeType.Element, TNodeType.ElementContainer, TNodeType.Container]);
   const registry = tView.directiveRegistry;
   let matches: any[]|null = null;
   if (registry) {
@@ -1377,6 +1378,12 @@ function findDirectiveDefMatches(
         diPublicInInjector(getOrCreateNodeInjectorForNode(tNode, viewData), tView, def.type);
 
         if (isComponentDef(def)) {
+          ngDevMode &&
+              assertNodeOfPossibleTypes(
+                  tNode, [TNodeType.Element],
+                  `"${tNode.tagName}" tags cannot be used as component hosts. ` +
+                      `Please use a different tag to activate the ${
+                          stringify(def.type)} component.`);
           if (tNode.flags & TNodeFlags.isComponentHost) throwMultipleComponentError(tNode);
           markAsComponentHost(tView, tNode);
           // The component is always stored first with directives after.
diff --git a/packages/core/src/render3/node_assert.ts b/packages/core/src/render3/node_assert.ts
@@ -26,12 +26,14 @@ export function assertNodeType(tNode: TNode, type: TNodeType): asserts tNode is
   assertEqual(tNode.type, type, `should be a ${typeName(type)}`);
 }
 
-export function assertNodeOfPossibleTypes(tNode: TNode|null, ...types: TNodeType[]): void {
+export function assertNodeOfPossibleTypes(
+    tNode: TNode|null, types: TNodeType[], message?: string): void {
   assertDefined(tNode, 'should be called with a TNode');
   const found = types.some(type => tNode.type === type);
   assertEqual(
       found, true,
-      `Should be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`);
+      message ??
+          `Should be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`);
 }
 
 export function assertNodeNotOfTypes(tNode: TNode, types: TNodeType[], message?: string): void {
diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -552,7 +552,7 @@ function getRenderParent(tView: TView, tNode: TNode, currentView: LView): REleme
     } else {
       // We are inserting a root element of the component view into the component host element and
       // it should always be eager.
-      ngDevMode && assertNodeOfPossibleTypes(hostTNode, TNodeType.Element);
+      ngDevMode && assertNodeOfPossibleTypes(hostTNode, [TNodeType.Element]);
       return currentView[HOST];
     }
   } else {
@@ -698,10 +698,10 @@ export function appendChild(
  */
 function getFirstNativeNode(lView: LView, tNode: TNode|null): RNode|null {
   if (tNode !== null) {
-    ngDevMode &&
-        assertNodeOfPossibleTypes(
-            tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer,
-            TNodeType.IcuContainer, TNodeType.Projection);
+    ngDevMode && assertNodeOfPossibleTypes(tNode, [
+      TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer, TNodeType.IcuContainer,
+      TNodeType.Projection
+    ]);
 
     const tNodeType = tNode.type;
     if (tNodeType === TNodeType.Element) {
@@ -778,10 +778,10 @@ function applyNodes(
     renderParent: RElement|null, beforeNode: RNode|null, isProjection: boolean) {
   while (tNode != null) {
     ngDevMode && assertTNodeForLView(tNode, lView);
-    ngDevMode &&
-        assertNodeOfPossibleTypes(
-            tNode, TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer,
-            TNodeType.Projection, TNodeType.Projection, TNodeType.IcuContainer);
+    ngDevMode && assertNodeOfPossibleTypes(tNode, [
+      TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer, TNodeType.Projection,
+      TNodeType.IcuContainer
+    ]);
     const rawSlotValue = lView[tNode.index];
     const tNodeType = tNode.type;
     if (isProjection) {
@@ -798,7 +798,7 @@ function applyNodes(
         applyProjectionRecursive(
             renderer, action, lView, tNode as TProjectionNode, renderParent, beforeNode);
       } else {
-        ngDevMode && assertNodeOfPossibleTypes(tNode, TNodeType.Element, TNodeType.Container);
+        ngDevMode && assertNodeOfPossibleTypes(tNode, [TNodeType.Element, TNodeType.Container]);
         applyToElementOrContainer(action, renderer, renderParent, rawSlotValue, beforeNode);
       }
     }
diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts
@@ -326,7 +326,7 @@ function createSpecialToken(lView: LView, tNode: TNode, read: any): any {
   } else if (read === ViewContainerRef) {
     ngDevMode &&
         assertNodeOfPossibleTypes(
-            tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer);
+            tNode, [TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer]);
     return createContainerRef(
         ViewContainerRef, ViewEngine_ElementRef,
         tNode as TElementNode | TContainerNode | TElementContainerNode, lView);
diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts
@@ -340,7 +340,7 @@ export function createContainerRef(
 
   ngDevMode &&
       assertNodeOfPossibleTypes(
-          hostTNode, TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer);
+          hostTNode, [TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer]);
 
   let lContainer: LContainer;
   const slotValue = hostView[hostTNode.index];
diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts
@@ -324,10 +324,10 @@ function collectNativeNodes(
     tView: TView, lView: LView, tNode: TNode|null, result: any[],
     isProjection: boolean = false): any[] {
   while (tNode !== null) {
-    ngDevMode &&
-        assertNodeOfPossibleTypes(
-            tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection,
-            TNodeType.ElementContainer, TNodeType.IcuContainer);
+    ngDevMode && assertNodeOfPossibleTypes(tNode, [
+      TNodeType.Element, TNodeType.Container, TNodeType.Projection, TNodeType.ElementContainer,
+      TNodeType.IcuContainer
+    ]);
 
     const lNode = lView[tNode.index];
     if (lNode !== null) {
diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts
@@ -11,7 +11,7 @@ import {ApplicationRef, Component, ComponentFactoryResolver, ComponentRef, Eleme
 import {TestBed} from '@angular/core/testing';
 import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
-import {onlyInIvy} from '@angular/private/testing';
+import {ivyEnabled, onlyInIvy} from '@angular/private/testing';
 
 import {domRendererFactory3} from '../../src/render3/interfaces/renderer';
 
@@ -259,6 +259,65 @@ describe('component', () => {
        expect(wrapperEls.length).toBe(2);  // other elements are preserved
      });
 
+  describe('invalid host element', () => {
+    it('should throw when <ng-container> is used as a host element for a Component', () => {
+      @Component({
+        selector: 'ng-container',
+        template: '...',
+      })
+      class Comp {
+      }
+
+      @Component({
+        selector: 'root',
+        template: '<ng-container></ng-container>',
+      })
+      class App {
+      }
+
+      TestBed.configureTestingModule({declarations: [App, Comp]});
+      if (ivyEnabled) {
+        expect(() => TestBed.createComponent(App))
+            .toThrowError(
+                /"ng-container" tags cannot be used as component hosts. Please use a different tag to activate the Comp component/);
+      } else {
+        // In VE there is no special check for the case when `<ng-container>` is used as a host
+        // element for a Component. VE tries to attach Component's content to a Comment node that
+        // represents the `<ng-container>` location and this call fails with a
+        // browser/environment-specific error message, so we just verify that this scenario is
+        // triggering an error in VE.
+        expect(() => TestBed.createComponent(App)).toThrow();
+      }
+    });
+
+    it('should throw when <ng-template> is used as a host element for a Component', () => {
+      @Component({
+        selector: 'ng-template',
+        template: '...',
+      })
+      class Comp {
+      }
+
+      @Component({
+        selector: 'root',
+        template: '<ng-template></ng-template>',
+      })
+      class App {
+      }
+
+      TestBed.configureTestingModule({declarations: [App, Comp]});
+      if (ivyEnabled) {
+        expect(() => TestBed.createComponent(App))
+            .toThrowError(
+                /"ng-template" tags cannot be used as component hosts. Please use a different tag to activate the Comp component/);
+      } else {
+        expect(() => TestBed.createComponent(App))
+            .toThrowError(
+                /Components on an embedded template: Comp \("\[ERROR ->\]<ng-template><\/ng-template>"\)/);
+      }
+    });
+  });
+
   it('should use a new ngcontent attribute for child elements created w/ Renderer2', () => {
     @Component({
       selector: 'app-root',

Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@`
`62`	`62`	`"bundle": "TODO(i): we should define ngDevMode to false in Closure, but --define only works in the global scope.",`
`63`	`63`	`"bundle": "TODO(i): (FW-2164) TS 3.9 new class shape seems to have broken Closure in big ways. The size went from 169991 to 252338",`
`64`	`64`	`"bundle": "TODO(i): after removal of tsickle from ngc-wrapped / ng_package, we had to switch to SIMPLE optimizations which increased the size from 252338 to 1198917, see PR#37221 and PR#37317 for more info",`
`65`		`- "bundle": 1209688`
	`65`	`+ "bundle": 1210239`
`66`	`66`	`}`
`67`	`67`	`}`
`68`	`68`	`}`