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.

Sign up for GitHub

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

Jump to bottom

perf(ivy): avoid generating selectors array for directives without a selector #33431

Closed
wants to merge 1 commit into from
Closed

perf(ivy): avoid generating selectors array for directives without a selector #33431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
}

Copy link
Member

Choose a reason for hiding this comment

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

Was this just dead-code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's been there for a while so I decided to clean it up.

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
Open in desktop
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
Open in desktop
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
Open in desktop
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