Skip to content

Commit

Permalink
perf(ivy): avoid generating selectors array for directives without a …
Browse files Browse the repository at this point in the history 
…selector (#33431)

Now that we've replaced `ngBaseDef` with an abstract directive definition, there are a lot more cases where we generate a directive definition without a selector. These changes make it so that we don't generate the `selectors` array if it's going to be empty.

PR Close #33431
  • Loading branch information
@crisbeto @AndrewKushnir
crisbeto authored and AndrewKushnir committed Oct 29, 2019
1 parent d1246a1 commit c3e9356
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 42 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts
View file
Expand Up @@ -310,7 +310,7 @@ A.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: A, selectors: [["", "a", ""]] });
expect(addDefinitionsSpy.calls.first().args[2])
.toEqual(
`UndecoratedBase.ɵfac = function UndecoratedBase_Factory(t) { return new (t || UndecoratedBase)(); };
UndecoratedBase.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, selectors: [], viewQuery: function UndecoratedBase_Query(rf, ctx) { if (rf & 1) {
UndecoratedBase.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, viewQuery: function UndecoratedBase_Query(rf, ctx) { if (rf & 1) {
ɵngcc0.ɵɵstaticViewQuery(_c0, true);
} if (rf & 2) {
var _t;
Expand Down
57 changes: 34 additions & 23 deletions packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts
View file
Expand Up @@ -2958,6 +2958,28 @@ describe('compiler compliance', () => {
expect(() => compile(files, angularFiles)).not.toThrow();
});

it('should not generate a selectors array if the directive does not have a selector', () => {
const files = {
app: {
'spec.ts': `
import {Directive} from '@angular/core';
@Directive()
export class AbstractDirective {
}
`
}
};
const expectedOutput = `
// ...
AbstractDirective.ɵdir = $r3$.ɵɵdefineDirective({
type: AbstractDirective
});
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

});

Expand Down Expand Up @@ -3004,7 +3026,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
inputs: {
input1: "input1",
input2: ["alias2", "input2"]
Expand All @@ -3013,7 +3034,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if one or more @Output is present', () => {
Expand Down Expand Up @@ -3052,7 +3073,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
outputs: {
output1: "output1",
output2: "output2"
Expand All @@ -3061,7 +3081,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a mixture of @Input and @Output props are present',
Expand Down Expand Up @@ -3107,7 +3127,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
inputs: {
input1: "input1",
input2: ["whatever", "input2"]
Expand All @@ -3120,7 +3139,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a ViewChild query is present', () => {
Expand Down Expand Up @@ -3151,7 +3170,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
viewQuery: function BaseClass_Query(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵviewQuery($e0_attrs$, true);
Expand All @@ -3165,7 +3183,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a ViewChildren query is present', () => {
Expand Down Expand Up @@ -3198,7 +3216,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
viewQuery: function BaseClass_Query(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵviewQuery(SomeDirective, true);
Expand All @@ -3212,7 +3229,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a ContentChild query is present', () => {
Expand Down Expand Up @@ -3243,7 +3260,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
contentQueries: function BaseClass_ContentQueries(rf, ctx, dirIndex) {
if (rf & 1) {
$r3$.ɵɵcontentQuery(dirIndex, $e0_attrs$, true);
Expand All @@ -3257,7 +3273,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a ContentChildren query is present', () => {
Expand Down Expand Up @@ -3290,7 +3306,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
contentQueries: function BaseClass_ContentQueries(rf, ctx, dirIndex) {
if (rf & 1) {
$r3$.ɵɵcontentQuery(dirIndex, SomeDirective, false);
Expand All @@ -3304,7 +3319,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a host binding is present', () => {
Expand Down Expand Up @@ -3335,7 +3350,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
hostBindings: function BaseClass_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵallocHostVars(1);
Expand All @@ -3348,7 +3362,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive if a host listener is present', () => {
Expand Down Expand Up @@ -3379,7 +3393,6 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
hostBindings: function BaseClass_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵlistener("mousedown", function BaseClass_mousedown_HostBindingHandler($event) {
Expand All @@ -3391,7 +3404,7 @@ describe('compiler compliance', () => {
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should add an abstract directive when using any lifecycle hook', () => {
Expand Down Expand Up @@ -3420,13 +3433,12 @@ describe('compiler compliance', () => {
const expectedOutput = `
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: []
type: BaseClass
});
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});


Expand Down Expand Up @@ -3457,13 +3469,12 @@ describe('compiler compliance', () => {
// ...
BaseClass.ɵdir = $r3$.ɵɵdefineDirective({
type: BaseClass,
selectors: [],
features: [$r3$.ɵɵNgOnChangesFeature()]
});
// ...
`;
const result = compile(files, angularFiles);
expectEmit(result.source, expectedOutput, 'Invalid base definition');
expectEmit(result.source, expectedOutput, 'Invalid directive definition');
});

it('should NOT add an abstract directive if @Component is present', () => {
Expand Down
3 changes: 1 addition & 2 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View file
Expand Up @@ -516,8 +516,7 @@ runInEachFileSystem(os => {

const jsContents = env.getContents('test.js');
expect(jsContents)
.toContain(
'i0.ɵɵdefineDirective({ type: TestBase, selectors: [], inputs: { input: "input" } });');
.toContain('i0.ɵɵdefineDirective({ type: TestBase, inputs: { input: "input" } });');

const dtsContents = env.getContents('test.d.ts');
expect(dtsContents)
Expand Down
14 changes: 4 additions & 10 deletions packages/compiler/src/render3/view/compiler.ts
View file
Expand Up @@ -38,20 +38,19 @@ const EMPTY_ARRAY: any[] = [];
// If there is a match, the first matching group will contain the attribute name to bind.
const ATTR_REGEX = /attr\.([^\]]+)/;

function getStylingPrefix(name: string): string {
return name.substring(0, 5); // style or class
}

function baseDirectiveFields(
meta: R3DirectiveMetadata, constantPool: ConstantPool,
bindingParser: BindingParser): DefinitionMap {
const definitionMap = new DefinitionMap();
const selectors = core.parseSelectorToR3Selector(meta.selector);

// e.g. `type: MyDirective`
definitionMap.set('type', meta.type);

// e.g. `selectors: [['', 'someDir', '']]`
definitionMap.set('selectors', createDirectiveSelector(meta.selector));
if (selectors.length > 0) {
definitionMap.set('selectors', asLiteral(selectors));
}

if (meta.queries.length > 0) {
// e.g. `contentQueries: (rf, ctx, dirIndex) => { ... }
Expand Down Expand Up @@ -406,11 +405,6 @@ function prepareQueryParams(query: R3QueryMetadata, constantPool: ConstantPool):
return parameters;
}

// Turn a directive selector into an R3-compatible selector for directive def
function createDirectiveSelector(selector: string | null): o.Expression {
return asLiteral(core.parseSelectorToR3Selector(selector));
}

function convertAttributesToExpressions(attributes: {[name: string]: o.Expression}):
o.Expression[] {
const values: o.Expression[] = [];
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/render3/definition.ts
View file
Expand Up @@ -48,7 +48,7 @@ export function ɵɵdefineComponent<T>(componentDefinition: {
type: Type<T>;

/** The selectors that will be used to match nodes to this component. */
selectors: CssSelectorList;
selectors?: CssSelectorList;

/**
* The number of nodes, local refs, and pipes in this component template.
Expand Down Expand Up @@ -275,7 +275,7 @@ export function ɵɵdefineComponent<T>(componentDefinition: {
onPush: componentDefinition.changeDetection === ChangeDetectionStrategy.OnPush,
directiveDefs: null !, // assigned in noSideEffects
pipeDefs: null !, // assigned in noSideEffects
selectors: componentDefinition.selectors,
selectors: componentDefinition.selectors || EMPTY_ARRAY,
viewQuery: componentDefinition.viewQuery || null,
features: componentDefinition.features as DirectiveDefFeature[] || null,
data: componentDefinition.data || {},
Expand Down Expand Up @@ -507,7 +507,7 @@ export const ɵɵdefineDirective = ɵɵdefineComponent as any as<T>(directiveDef
type: Type<T>;

/** The selectors that will be used to match nodes to this directive. */
selectors: CssSelectorList;
selectors?: CssSelectorList;

/**
* A map of input names.
Expand Down
1 change: 0 additions & 1 deletion packages/core/test/render3/render_util.ts
View file
Expand Up @@ -261,7 +261,6 @@ export function renderTemplate<T>(
enterView(hostLView, null);

const def: ComponentDef<any> = ɵɵdefineComponent({
selectors: [],
type: Object,
template: templateFn,
decls: decls,
Expand Down
4 changes: 2 additions & 2 deletions tools/public_api_guard/core/core.d.ts
View file
Expand Up @@ -747,7 +747,7 @@ export declare const ɵɵdefaultStyleSanitizer: StyleSanitizeFn;

export declare function ɵɵdefineComponent<T>(componentDefinition: {
type: Type<T>;
selectors: CssSelectorList;
selectors?: CssSelectorList;
decls: number;
vars: number;
inputs?: {
Expand Down Expand Up @@ -777,7 +777,7 @@ export declare function ɵɵdefineComponent<T>(componentDefinition: {

export declare const ɵɵdefineDirective: <T>(directiveDefinition: {
type: Type<T>;
selectors: (string | SelectorFlags)[][];
selectors?: (string | SelectorFlags)[][] | undefined;
inputs?: { [P in keyof T]?: string | [string, string] | undefined; } | undefined;
outputs?: { [P_1 in keyof T]?: string | undefined; } | undefined;
features?: DirectiveDefFeature[] | undefined;
Expand Down

0 comments on commit c3e9356

Please sign in to comment.