From fdbe9f5d9ffb84b34093c1bd5d10cabf76e58bb2 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 14 May 2020 09:34:51 -0700 Subject: [PATCH] refactor(core): assert TNode is not a container when setting attribute on element (#37111) This PR provides a more helpful error than the one currently present: `el.setAttribute is not a function`. It is not valid to have directives with host bindings on `ng-template` or `ng-container` nodes. VE would silently ignore this, while Ivy attempts to set the attribute and throws an error because these are comment nodes and do not have `setAttribute` functionality. It is better to throw a helpful error than to silently ignore this because putting a directive with host binding on an `ng-template` or `ng-container` is most often a mistake. Developers should be made aware that the host binding will have no effect in these cases. Note that an error is already thrown in Ivy, as mentioned above, so this is not a breaking change and can be merged to both master and patch. Resolves #35994 PR Close #37111 --- .../size-tracking/integration-payloads.json | 2 +- .../core/src/render3/instructions/shared.ts | 12 +++-- packages/core/src/render3/node_assert.ts | 9 ++++ .../core/test/acceptance/host_binding_spec.ts | 49 +++++++++++++++++++ 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 7e65ea3e75..0caded8a40 100644 --- 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 1197869, see PR#37221 and PR#37317 for more info", - "bundle": 1197869 + "bundle": 1198831 } } } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 535e89fc77..1ca8a714d7 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -29,7 +29,7 @@ import {isProceduralRenderer, RComment, RElement, Renderer3, RendererFactory3, R import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TVIEW, TView, TViewType} from '../interfaces/view'; -import {assertNodeOfPossibleTypes} from '../node_assert'; +import {assertNodeNotOfTypes, assertNodeOfPossibleTypes} from '../node_assert'; import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher'; import {enterView, getBindingsEnabled, getCheckNoChangesMode, getCurrentDirectiveIndex, getIsParent, getPreviousOrParentTNode, getSelectedIndex, leaveView, setBindingIndex, setBindingRootForHostBindings, setCheckNoChangesMode, setCurrentDirectiveIndex, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; import {NO_CHANGE} from '../tokens'; @@ -1484,8 +1484,14 @@ function addComponentLogic(lView: LView, hostTNode: TElementNode, def: Compon export function elementAttributeInternal( tNode: TNode, lView: LView, name: string, value: any, sanitizer: SanitizerFn|null|undefined, namespace: string|null|undefined) { - ngDevMode && assertNotSame(value, NO_CHANGE as any, 'Incoming value should never be NO_CHANGE.'); - ngDevMode && validateAgainstEventAttributes(name); + if (ngDevMode) { + assertNotSame(value, NO_CHANGE as any, 'Incoming value should never be NO_CHANGE.'); + validateAgainstEventAttributes(name); + assertNodeNotOfTypes( + tNode, [TNodeType.Container, TNodeType.ElementContainer], + `Attempted to set attribute \`${name}\` on a container node. ` + + `Host bindings are not valid on ng-container or ng-template.`); + } const element = getNativeByTNode(tNode, lView) as RElement; const renderer = lView[RENDERER]; if (value == null) { diff --git a/packages/core/src/render3/node_assert.ts b/packages/core/src/render3/node_assert.ts index 0b801ba62b..895c52397d 100644 --- a/packages/core/src/render3/node_assert.ts +++ b/packages/core/src/render3/node_assert.ts @@ -34,6 +34,15 @@ export function assertNodeOfPossibleTypes(tNode: TNode|null, ...types: TNodeType `Should be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`); } +export function assertNodeNotOfTypes(tNode: TNode, types: TNodeType[], message?: string): void { + assertDefined(tNode, 'should be called with a TNode'); + const found = types.some(type => tNode.type === type); + assertEqual( + found, false, + message ?? + `Should not be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`); +} + function typeName(type: TNodeType): string { if (type == TNodeType.Projection) return 'Projection'; if (type == TNodeType.Container) return 'Container'; diff --git a/packages/core/test/acceptance/host_binding_spec.ts b/packages/core/test/acceptance/host_binding_spec.ts index e030432a21..27726e66bf 100644 --- a/packages/core/test/acceptance/host_binding_spec.ts +++ b/packages/core/test/acceptance/host_binding_spec.ts @@ -1427,4 +1427,53 @@ describe('host bindings', () => { '', bypassSanitizationTrustHtml, /* isAttribute */ false); }); + + onlyInIvy('VE would silently ignore this').describe('host binding on containers', () => { + @Directive({selector: '[staticHostAtt]', host: {'static': 'attr'}}) + class StaticHostAttr { + constructor() {} + } + + @Directive({selector: '[dynamicHostAtt]', host: {'[attr.dynamic]': '"dynamic"'}}) + class DynamicHostAttr { + constructor() {} + } + + it('should fail with expected error with ng-container', () => { + @Component({ + selector: 'my-app', + template: ` + + + ` + }) + class App { + } + + const comp = + TestBed.configureTestingModule({declarations: [App, StaticHostAttr, DynamicHostAttr]}) + .createComponent(App); + // TODO(FW-2202): binding static attrs won't throw an error. We should be more consistent. + expect(() => comp.detectChanges()) + .toThrowError( + /Attempted to set attribute `dynamic` on a container node. Host bindings are not valid on ng-container or ng-template./); + }); + + it('should fail with expected error with ng-template', () => { + @Component({ + selector: 'my-app', + template: ` ` + }) + class App { + } + + const comp = + TestBed.configureTestingModule({declarations: [App, StaticHostAttr, DynamicHostAttr]}) + .createComponent(App); + // TODO(FW-2202): binding static attrs won't throw an error. We should be more consistent. + expect(() => comp.detectChanges()) + .toThrowError( + /Attempted to set attribute `dynamic` on a container node. Host bindings are not valid on ng-container or ng-template./); + }); + }); });