From c83f5013bfb0182dc653d7e5d8a1750e09ea50e9 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 26 Oct 2019 12:44:03 +0200 Subject: [PATCH] feat(ivy): implement unknown element detection in jit mode (#33419) In ViewEngine we used to throw an error if we encountered an unknown element while rendering. We have this already for Ivy in AoT, but we didn't in JiT. These changes implement the error for JiT mode. PR Close #33419 --- .../core/src/render3/instructions/element.ts | 37 +++++++- .../core/src/render3/instructions/shared.ts | 9 +- packages/core/test/acceptance/i18n_spec.ts | 10 ++- .../core/test/acceptance/ng_module_spec.ts | 84 ++++++++++++++++++- .../test/linker/ng_module_integration_spec.ts | 2 +- .../test/testing_public_spec.ts | 2 +- 6 files changed, 132 insertions(+), 12 deletions(-) diff --git a/packages/core/src/render3/instructions/element.ts b/packages/core/src/render3/instructions/element.ts index c73cb8b03814b..b8be8c4e60064 100644 --- a/packages/core/src/render3/instructions/element.ts +++ b/packages/core/src/render3/instructions/element.ts @@ -10,7 +10,7 @@ import {assertDataInRange, assertDefined, assertEqual} from '../../util/assert'; import {assertHasParent} from '../assert'; import {attachPatchData} from '../context_discovery'; import {registerPostOrderHooks} from '../hooks'; -import {TAttributes, TNodeFlags, TNodeType} from '../interfaces/node'; +import {TAttributes, TNode, TNodeFlags, TNodeType} from '../interfaces/node'; import {RElement} from '../interfaces/renderer'; import {StylingMapArray, TStylingContext} from '../interfaces/styling'; import {isContentQueryHost, isDirectiveHost} from '../interfaces/type_checks'; @@ -22,7 +22,7 @@ import {setUpAttributes} from '../util/attrs_utils'; import {getInitialStylingValue, hasClassInput, hasStyleInput, selectClassBasedInputName} from '../util/styling_utils'; import {getNativeByTNode, getTNode} from '../util/view_utils'; -import {createDirectivesInstances, elementCreate, executeContentQueries, getOrCreateTNode, renderInitialStyling, resolveDirectives, saveResolvedLocalsInData, setInputsForProperty} from './shared'; +import {createDirectivesInstances, elementCreate, executeContentQueries, getOrCreateTNode, matchingSchemas, renderInitialStyling, resolveDirectives, saveResolvedLocalsInData, setInputsForProperty} from './shared'; import {registerInitialStylingOnTNode} from './styling'; @@ -84,7 +84,8 @@ export function ɵɵelementStart( // and `[class]` bindings work for multiple directives.) if (tView.firstTemplatePass) { ngDevMode && ngDevMode.firstTemplatePass++; - resolveDirectives(tView, lView, tNode, localRefs || null); + const hasDirectives = resolveDirectives(tView, lView, tNode, localRefs || null); + ngDevMode && validateElement(lView, native, tNode, hasDirectives); if (tView.queries !== null) { tView.queries.elementStart(tView, tNode); @@ -241,3 +242,33 @@ function setDirectiveStylingInput( // be (Jira Issue = FW-1467). setInputsForProperty(lView, stylingInputs, value); } + +function validateElement( + hostView: LView, element: RElement, tNode: TNode, hasDirectives: boolean): void { + const tagName = tNode.tagName; + + // If the element matches any directive, it's considered as valid. + if (!hasDirectives && tagName !== null) { + // The element is unknown if it's an instance of HTMLUnknownElement or it isn't registered + // as a custom element. Note that unknown elements with a dash in their name won't be instances + // of HTMLUnknownElement in browsers that support web components. + const isUnknown = + (typeof HTMLUnknownElement === 'function' && element instanceof HTMLUnknownElement) || + (typeof customElements !== 'undefined' && tagName.indexOf('-') > -1 && + !customElements.get(tagName)); + + if (isUnknown && !matchingSchemas(hostView, tagName)) { + let errorMessage = `'${tagName}' is not a known element:\n`; + errorMessage += + `1. If '${tagName}' is an Angular component, then verify that it is part of this module.\n`; + if (tagName && tagName.indexOf('-') > -1) { + errorMessage += + `2. If '${tagName}' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@NgModule.schemas' of this component to suppress this message.`; + } else { + errorMessage += + `2. To allow any element add 'NO_ERRORS_SCHEMA' to the '@NgModule.schemas' of this component.`; + } + throw new Error(errorMessage); + } + } +} diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index c5cd3cde6d266..078f353235bbf 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -989,7 +989,7 @@ function validateProperty( isAnimationProp(propName) || typeof Node !== 'function' || !(element instanceof Node); } -function matchingSchemas(hostView: LView, tagName: string | null): boolean { +export function matchingSchemas(hostView: LView, tagName: string | null): boolean { const schemas = hostView[TVIEW].schemas; if (schemas !== null) { @@ -1040,17 +1040,19 @@ export function instantiateRootComponent(tView: TView, lView: LView, def: Com */ export function resolveDirectives( tView: TView, lView: LView, tNode: TElementNode | TContainerNode | TElementContainerNode, - localRefs: string[] | null): void { + localRefs: string[] | null): boolean { // Please make sure to have explicit type for `exportsMap`. Inferred type triggers bug in // tsickle. ngDevMode && assertFirstTemplatePass(tView); - if (!getBindingsEnabled()) return; + if (!getBindingsEnabled()) return false; const directives: DirectiveDef[]|null = findDirectiveMatches(tView, lView, tNode); const exportsMap: ({[key: string]: number} | null) = localRefs ? {'': -1} : null; + let hasDirectives = false; if (directives !== null) { + hasDirectives = true; initNodeFlags(tNode, tView.data.length, directives.length); // When the same token is provided by several directives on the same node, some rules apply in // the viewEngine: @@ -1088,6 +1090,7 @@ export function resolveDirectives( initializeInputAndOutputAliases(tView, tNode); } if (exportsMap) cacheMatchingLocalNames(tNode, localRefs, exportsMap); + return hasDirectives; } /** diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts index 99b184896a2f9..afab8754963d1 100644 --- a/packages/core/test/acceptance/i18n_spec.ts +++ b/packages/core/test/acceptance/i18n_spec.ts @@ -10,7 +10,7 @@ import '@angular/localize/init'; import {registerLocaleData} from '@angular/common'; import localeRo from '@angular/common/locales/ro'; -import {Component, ContentChild, ContentChildren, Directive, HostBinding, Input, LOCALE_ID, QueryList, TemplateRef, Type, ViewChild, ViewContainerRef, Pipe, PipeTransform} from '@angular/core'; +import {Component, ContentChild, ContentChildren, Directive, HostBinding, Input, LOCALE_ID, QueryList, TemplateRef, Type, ViewChild, ViewContainerRef, Pipe, PipeTransform, NO_ERRORS_SCHEMA} from '@angular/core'; import {setDelayProjection} from '@angular/core/src/render3/instructions/projection'; import {TestBed} from '@angular/core/testing'; import {loadTranslations, clearTranslations} from '@angular/localize'; @@ -22,7 +22,13 @@ import {computeMsgId} from '@angular/compiler'; onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => { beforeEach(() => { - TestBed.configureTestingModule({declarations: [AppComp, DirectiveWithTplRef, UppercasePipe]}); + TestBed.configureTestingModule({ + declarations: [AppComp, DirectiveWithTplRef, UppercasePipe], + // In some of the tests we use made-up tag names for better readability, however they'll + // cause validation errors. Add the `NO_ERRORS_SCHEMA` so that we don't have to declare + // dummy components for each one of them. + schemas: [NO_ERRORS_SCHEMA], + }); }); afterEach(() => { diff --git a/packages/core/test/acceptance/ng_module_spec.ts b/packages/core/test/acceptance/ng_module_spec.ts index 4258f09edbb89..09c4c96cff276 100644 --- a/packages/core/test/acceptance/ng_module_spec.ts +++ b/packages/core/test/acceptance/ng_module_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {Component, NO_ERRORS_SCHEMA, NgModule} from '@angular/core'; +import {CUSTOM_ELEMENTS_SCHEMA, Component, NO_ERRORS_SCHEMA, NgModule} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {modifiedInIvy, onlyInIvy} from '@angular/private/testing'; @@ -164,6 +164,86 @@ describe('NgModule', () => { fixture.detectChanges(); }).not.toThrow(); }); - }); + it('should throw unknown element error without CUSTOM_ELEMENTS_SCHEMA for element with dash in tag name', + () => { + @Component({template: ``}) + class MyComp { + } + + TestBed.configureTestingModule({declarations: [MyComp]}); + + expect(() => { + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + }).toThrowError(/'custom-el' is not a known element/); + }); + + it('should throw unknown element error without CUSTOM_ELEMENTS_SCHEMA for element without dash in tag name', + () => { + @Component({template: ``}) + class MyComp { + } + + TestBed.configureTestingModule({declarations: [MyComp]}); + + expect(() => { + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + }).toThrowError(/'custom' is not a known element/); + }); + + it('should not throw unknown element error with CUSTOM_ELEMENTS_SCHEMA', () => { + @Component({template: ``}) + class MyComp { + } + + TestBed.configureTestingModule({ + declarations: [MyComp], + schemas: [CUSTOM_ELEMENTS_SCHEMA], + }); + + expect(() => { + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + }).not.toThrow(); + }); + + it('should not throw unknown element error with NO_ERRORS_SCHEMA', () => { + @Component({template: ``}) + class MyComp { + } + + TestBed.configureTestingModule({ + declarations: [MyComp], + schemas: [NO_ERRORS_SCHEMA], + }); + + expect(() => { + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + }).not.toThrow(); + }); + + it('should not throw unknown element error if element matches a directive', () => { + @Component({ + selector: 'custom-el', + template: '', + }) + class CustomEl { + } + + @Component({template: ``}) + class MyComp { + } + + TestBed.configureTestingModule({declarations: [MyComp, CustomEl]}); + + expect(() => { + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + }).not.toThrow(); + }); + + }); }); diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts index beef110c3a5fb..bdc52b9621fae 100644 --- a/packages/core/test/linker/ng_module_integration_spec.ts +++ b/packages/core/test/linker/ng_module_integration_spec.ts @@ -252,7 +252,7 @@ function declareTests(config?: {useJit: boolean}) { onlyInIvy('Unknown property warning logged, instead of throwing an error') .it('should error on unknown bound properties on custom elements by default', () => { - @Component({template: ''}) + @Component({template: '
'}) class ComponentUsingInvalidProperty { } diff --git a/packages/platform-browser/test/testing_public_spec.ts b/packages/platform-browser/test/testing_public_spec.ts index c5f97a861c0a4..24608a88bdef6 100644 --- a/packages/platform-browser/test/testing_public_spec.ts +++ b/packages/platform-browser/test/testing_public_spec.ts @@ -958,7 +958,7 @@ Did you run and wait for 'resolveComponentResources()'?` : onlyInIvy(`Unknown property warning logged instead of an error`) .it('should error on unknown bound properties on custom elements by default', () => { - @Component({template: ''}) + @Component({template: '
'}) class ComponentUsingInvalidProperty { }