Skip to content

Commit

Permalink
feat(ivy): implement unknown element detection in jit mode (#33419)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
crisbeto authored and atscott committed Nov 4, 2019
1 parent ac9d044 commit c83f501
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 12 deletions.
37 changes: 34 additions & 3 deletions packages/core/src/render3/instructions/element.ts
Expand Up @@ -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';
Expand All @@ -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';


Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}
}
9 changes: 6 additions & 3 deletions packages/core/src/render3/instructions/shared.ts
Expand Up @@ -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) {
Expand Down Expand Up @@ -1040,17 +1040,19 @@ export function instantiateRootComponent<T>(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<any>[]|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:
Expand Down Expand Up @@ -1088,6 +1090,7 @@ export function resolveDirectives(
initializeInputAndOutputAliases(tView, tNode);
}
if (exportsMap) cacheMatchingLocalNames(tNode, localRefs, exportsMap);
return hasDirectives;
}

/**
Expand Down
10 changes: 8 additions & 2 deletions packages/core/test/acceptance/i18n_spec.ts
Expand Up @@ -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';
Expand All @@ -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(() => {
Expand Down
84 changes: 82 additions & 2 deletions packages/core/test/acceptance/ng_module_spec.ts
Expand Up @@ -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';

Expand Down Expand Up @@ -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: `<custom-el></custom-el>`})
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: `<custom></custom>`})
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: `<custom-el></custom-el>`})
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: `<custom-el></custom-el>`})
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: `<custom-el></custom-el>`})
class MyComp {
}

TestBed.configureTestingModule({declarations: [MyComp, CustomEl]});

expect(() => {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
}).not.toThrow();
});

});
});
2 changes: 1 addition & 1 deletion packages/core/test/linker/ng_module_integration_spec.ts
Expand Up @@ -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: '<some-element [someUnknownProp]="true"></some-element>'})
@Component({template: '<div [someUnknownProp]="true"></div>'})
class ComponentUsingInvalidProperty {
}

Expand Down
2 changes: 1 addition & 1 deletion packages/platform-browser/test/testing_public_spec.ts
Expand Up @@ -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: '<some-element [someUnknownProp]="true"></some-element>'})
@Component({template: '<div [someUnknownProp]="true"></div>'})
class ComponentUsingInvalidProperty {
}

Expand Down

0 comments on commit c83f501

Please sign in to comment.