Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ivy): implement unknown element detection in jit mode #33419

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for reviewer: should we be throwing here or logging a warning? For the property validation we decided to warn.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be warning here. Unfortunately seeing this pretty late :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll submit another PR to switch it to a warning.

}
}
}
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