Skip to content

feat(ivy): emit module scope metadata using pure function call #29598

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

Closed
JoostK wants to merge 4 commits into from
Closed

feat(ivy): emit module scope metadata using pure function call #29598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
269b4f0
feat(ivy): emit module scope metadata using pure function call
JoostK Mar 29, 2019
75c56ed
feat(ivy): exclude declarations from injector imports
JoostK Mar 30, 2019
993809b
fixup! feat(ivy): exclude declarations from injector imports
JoostK Mar 30, 2019
9c94276
feat(ivy): do not emit empty providers/imports for defineInjector
JoostK Mar 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration/_payload-limits.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"master": {
"uncompressed": {
"runtime": 1440,
"main": 212976,
"main": 155609,
"polyfills": 43567
}
}
Expand Down
19 changes: 9 additions & 10 deletions packages/compiler-cli/ngcc/src/rendering/renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,16 +486,15 @@ export function renderDefinitions(
const name = compiledClass.declaration.name;
const translate = (stmt: Statement) =>
translateStatement(stmt, imports, NOOP_DEFAULT_IMPORT_RECORDER);
const definitions =
compiledClass.compilation
.map(
c => c.statements.map(statement => translate(statement))
.concat(translate(createAssignmentStatement(name, c.name, c.initializer)))
.map(
statement =>
printer.printNode(ts.EmitHint.Unspecified, statement, sourceFile))
.join('\n'))
.join('\n');
const print = (stmt: Statement) =>
printer.printNode(ts.EmitHint.Unspecified, translate(stmt), sourceFile);
const definitions = compiledClass.compilation
.map(
c => [createAssignmentStatement(name, c.name, c.initializer)]
.concat(c.statements)
.map(print)
.join('\n'))
.join('\n');
return definitions;
}

Expand Down
20 changes: 11 additions & 9 deletions packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,17 @@ describe('Renderer', () => {
moduleWithProvidersAnalyses);
const addDefinitionsSpy = renderer.addDefinitions as jasmine.Spy;
expect(addDefinitionsSpy.calls.first().args[2])
.toEqual(`/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(A, [{
type: Component,
args: [{ selector: 'a', template: '{{ person!.name }}' }]
}], null, null);
A.ngComponentDef = ɵngcc0.ɵdefineComponent({ type: A, selectors: [["a"]], factory: function A_Factory(t) { return new (t || A)(); }, consts: 1, vars: 1, template: function A_Template(rf, ctx) { if (rf & 1) {
.toEqual(
`A.ngComponentDef = ɵngcc0.ɵdefineComponent({ type: A, selectors: [["a"]], factory: function A_Factory(t) { return new (t || A)(); }, consts: 1, vars: 1, template: function A_Template(rf, ctx) { if (rf & 1) {
ɵngcc0.ɵtext(0);
} if (rf & 2) {
ɵngcc0.ɵselect(0);
ɵngcc0.ɵtextBinding(0, ɵngcc0.ɵinterpolation1("", ctx.person.name, ""));
} }, encapsulation: 2 });`);
} }, encapsulation: 2 });
/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(A, [{
type: Component,
args: [{ selector: 'a', template: '{{ person!.name }}' }]
}], null, null);`);
});


Expand Down Expand Up @@ -191,11 +192,12 @@ A.ngComponentDef = ɵngcc0.ɵdefineComponent({ type: A, selectors: [["a"]], fact
decorators: [jasmine.objectContaining({name: 'Directive'})],
}));
expect(addDefinitionsSpy.calls.first().args[2])
.toEqual(`/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(A, [{
.toEqual(
`A.ngDirectiveDef = ɵngcc0.ɵdefineDirective({ type: A, selectors: [["", "a", ""]], factory: function A_Factory(t) { return new (t || A)(); } });
/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(A, [{
type: Directive,
args: [{ selector: '[a]' }]
}], null, { foo: [] });
A.ngDirectiveDef = ɵngcc0.ɵdefineDirective({ type: A, selectors: [["", "a", ""]], factory: function A_Factory(t) { return new (t || A)(); } });`);
}], null, { foo: [] });`);
});

it('should call removeDecorators with the source code, a map of class decorators that have been analyzed',
Expand Down
33 changes: 25 additions & 8 deletions packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {DefaultImportRecorder, Reference, ReferenceEmitter} from '../../imports'
import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator';
import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral, typeNodeToValueExpr} from '../../reflection';
import {NgModuleRouteAnalyzer} from '../../routing';
import {LocalModuleScopeRegistry} from '../../scope';
import {LocalModuleScopeRegistry, ScopeData} from '../../scope';
import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform';
import {getSourceFile} from '../../util/src/typescript';

Expand All @@ -27,6 +27,7 @@ export interface NgModuleAnalysis {
ngInjectorDef: R3InjectorMetadata;
metadataStmt: Statement|null;
declarations: Reference<ClassDeclaration>[];
exports: Reference<ClassDeclaration>[];
}

/**
Expand Down Expand Up @@ -157,18 +158,16 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
schemas: [],
};

const providers: Expression = ngModule.has('providers') ?
new WrappedNodeExpr(ngModule.get('providers') !) :
new LiteralArrayExpr([]);
const rawProviders = ngModule.has('providers') ? ngModule.get('providers') ! : null;
const providers = rawProviders !== null ? new WrappedNodeExpr(rawProviders) : null;

// At this point, only add the module's imports as the injectors' imports. Any exported modules
// are added during `resolve`, as we need scope information to be able to filter out directives
// and pipes from the module exports.
const injectorImports: WrappedNodeExpr<ts.Expression>[] = [];
if (ngModule.has('imports')) {
injectorImports.push(new WrappedNodeExpr(ngModule.get('imports') !));
}
if (ngModule.has('exports')) {
injectorImports.push(new WrappedNodeExpr(ngModule.get('exports') !));
}

if (this.routeAnalyzer !== null) {
this.routeAnalyzer.add(node.getSourceFile(), name, rawImports, rawExports, rawProviders);
Expand All @@ -180,14 +179,15 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
deps: getValidConstructorDependencies(
node, this.reflector, this.defaultImportRecorder, this.isCore),
providers,
imports: new LiteralArrayExpr(injectorImports),
imports: injectorImports,
};

return {
analysis: {
ngModuleDef,
ngInjectorDef,
declarations: declarationRefs,
exports: exportRefs,
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore),
},
Expand All @@ -198,6 +198,18 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
resolve(node: ClassDeclaration, analysis: NgModuleAnalysis): ResolveResult {
const scope = this.scopeRegistry.getScopeOfModule(node);
const diagnostics = this.scopeRegistry.getDiagnosticsOfModule(node) || undefined;

// Using the scope information, extend the injector's imports using the modules that are
// specified as module exports.
if (scope !== null) {
const context = getSourceFile(node);
for (const exportRef of analysis.exports) {
if (isNgModule(exportRef.node, scope.compilation)) {

@JoostK JoostK Mar 30, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am not too certain about using the module scope to figure out if the ref is a module. It does however conveniently provide us with information on both refs from the program, as well as resolved refs from typings files.

@alxhub alxhub Apr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I actually like the approach of filtering out any exports from the scope, instead of filtering and including only known modules. It feels more robust.

analysis.ngInjectorDef.imports.push(this.refEmitter.emit(exportRef, context));
}
}
}

if (scope === null || scope.reexports === null) {
return {diagnostics};
} else {
Expand Down Expand Up @@ -387,6 +399,11 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
}
}

function isNgModule(node: ClassDeclaration, compilation: ScopeData): boolean {
return !compilation.directives.some(directive => directive.ref.node === node) &&
!compilation.pipes.some(pipe => pipe.ref.node === node);
}

function isDeclarationReference(ref: any): ref is Reference<ts.Declaration> {
return ref instanceof Reference &&
(ts.isClassDeclaration(ref.node) || ts.isFunctionDeclaration(ref.node) ||
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/imports/src/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const CORE_SUPPORTED_SYMBOLS = new Map<string, string>([
['defineInjectable', 'defineInjectable'],
['defineInjector', 'defineInjector'],
['ɵdefineNgModule', 'defineNgModule'],
['ɵsetNgModuleScope', 'setNgModuleScope'],
['inject', 'inject'],
['ɵsetClassMetadata', 'setClassMetadata'],
['ɵInjectableDef', 'InjectableDef'],
Expand Down
89 changes: 84 additions & 5 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,13 @@ describe('ngtsc behavioral tests', () => {
env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule, bootstrap: [TestCmp] });');
expect(jsContents)
.toContain('/*@__PURE__*/ i0.ɵsetNgModuleScope(TestModule, { declarations: [TestCmp] });');
expect(jsContents)
.toContain(
'i0.ɵdefineNgModule({ type: TestModule, bootstrap: [TestCmp], ' +
'declarations: [TestCmp] })');
'i0.defineInjector({ factory: ' +
'function TestModule_Factory(t) { return new (t || TestModule)(); } });');

const dtsContents = env.getContents('test.d.ts');
expect(dtsContents)
Expand All @@ -457,6 +460,82 @@ describe('ngtsc behavioral tests', () => {
expect(dtsContents).not.toContain('__decorate');
});

it('should not emit a setNgModuleScope call when no scope metadata is present', () => {
env.tsconfig();
env.write('test.ts', `
import {NgModule} from '@angular/core';

@NgModule({})
export class TestModule {}
`);

env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule });');
expect(jsContents).not.toContain('ɵsetNgModuleScope(TestModule,');
});

it('should filter out directives and pipes from module exports in the injector def', () => {
env.tsconfig();
env.write('test.ts', `
import {NgModule} from '@angular/core';
import {RouterComp, RouterModule} from '@angular/router';
import {Dir, OtherDir, MyPipe, Comp} from './decls';

@NgModule({
declarations: [OtherDir],
exports: [OtherDir],
})
export class OtherModule {}

const EXPORTS = [Dir, MyPipe, Comp, OtherModule, OtherDir, RouterModule, RouterComp];

@NgModule({
declarations: [Dir, MyPipe, Comp],
imports: [OtherModule, RouterModule.forRoot()],
exports: [EXPORTS],
})
export class TestModule {}
`);
env.write(`decls.ts`, `
import {Component, Directive, Pipe} from '@angular/core';

@Directive({selector: '[dir]'})
export class Dir {}

@Directive({selector: '[other]'})
export class OtherDir {}

@Pipe({name:'pipe'})
export class MyPipe {}

@Component({selector: 'test', template: ''})
export class Comp {}
`);
env.write('node_modules/@angular/router/index.d.ts', `
import {ɵComponentDefWithMeta, ModuleWithProviders, ɵNgModuleDefWithMeta} from '@angular/core';

export declare class RouterComp {
static ngComponentDef: ɵComponentDefWithMeta<RouterComp, "lib-cmp", never, {}, {}, never>
}

declare class RouterModule {
static forRoot(): ModuleWithProviders<RouterModule>;
static ngModuleDef: ɵNgModuleDefWithMeta<RouterModule, [typeof RouterComp], never, [typeof RouterComp]>;
}
`);

env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents)
.toContain(

@alxhub alxhub Apr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to match the whole structure of defineInjector, it's enough to assert that it contains imports: [...]. You can also use a regex to ignore the whitespace.

'i0.defineInjector({ factory: function TestModule_Factory(t) ' +
'{ return new (t || TestModule)(); }, imports: [[OtherModule, RouterModule.forRoot()],' +
'\n OtherModule,\n RouterModule] });');

@alxhub alxhub Apr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using the ts.Printer, you're going to get whitespace attached to existing nodes.

});

it('should compile NgModules with services without errors', () => {
env.tsconfig();
env.write('test.ts', `
Expand Down Expand Up @@ -484,7 +563,7 @@ describe('ngtsc behavioral tests', () => {
env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule,');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule });');
expect(jsContents)
.toContain(
`TestModule.ngInjectorDef = i0.defineInjector({ factory: ` +
Expand Down Expand Up @@ -525,7 +604,7 @@ describe('ngtsc behavioral tests', () => {
env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule,');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule });');
expect(jsContents)
.toContain(
`TestModule.ngInjectorDef = i0.defineInjector({ factory: ` +
Expand Down Expand Up @@ -570,7 +649,7 @@ describe('ngtsc behavioral tests', () => {
env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule,');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule });');
expect(jsContents)
.toContain(
`TestModule.ngInjectorDef = i0.defineInjector({ factory: ` +
Expand Down
52 changes: 52 additions & 0 deletions packages/compiler-cli/test/ngtsc/scope_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,32 @@ describe('ngtsc module scopes', () => {

describe('diagnostics', () => {
describe('imports', () => {
it('should emit imports in a pure function call', () => {
env.tsconfig();
env.write('test.ts', `
import {NgModule} from '@angular/core';

@NgModule({})
export class OtherModule {}

@NgModule({imports: [OtherModule]})
export class TestModule {}
`);

env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule });');
expect(jsContents)
.toContain(
'/*@__PURE__*/ i0.ɵsetNgModuleScope(TestModule, { imports: [OtherModule] });');

const dtsContents = env.getContents('test.d.ts');
expect(dtsContents)
.toContain(
'static ngModuleDef: i0.ɵNgModuleDefWithMeta<TestModule, never, [typeof OtherModule], never>');
});

it('should produce an error when an invalid class is imported', () => {
env.write('test.ts', `
import {NgModule} from '@angular/core';
Expand Down Expand Up @@ -57,6 +83,32 @@ describe('ngtsc module scopes', () => {
});

describe('exports', () => {
it('should emit exports in a pure function call', () => {
env.tsconfig();
env.write('test.ts', `
import {NgModule} from '@angular/core';

@NgModule({})
export class OtherModule {}

@NgModule({exports: [OtherModule]})
export class TestModule {}
`);

env.driveMain();

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('i0.ɵdefineNgModule({ type: TestModule });');
expect(jsContents)
.toContain(
'/*@__PURE__*/ i0.ɵsetNgModuleScope(TestModule, { exports: [OtherModule] });');

const dtsContents = env.getContents('test.d.ts');
expect(dtsContents)
.toContain(
'static ngModuleDef: i0.ɵNgModuleDefWithMeta<TestModule, never, never, [typeof OtherModule]>');
});

it('should produce an error when a non-NgModule class is exported', () => {
env.write('test.ts', `
import {NgModule} from '@angular/core';
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/src/compiler_facade_interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ export interface R3InjectorMetadataFacade {
name: string;
type: any;
deps: R3DependencyMetadataFacade[]|null;
providers: any;
imports: any;
providers: any[];
imports: any[];
}

export interface R3DirectiveMetadataFacade {
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/jit_compiler_facade.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export class CompilerFacadeImpl implements CompilerFacade {
type: new WrappedNodeExpr(facade.type),
deps: convertR3DependencyMetadataArray(facade.deps),
providers: new WrappedNodeExpr(facade.providers),
imports: new WrappedNodeExpr(facade.imports),
imports: facade.imports.map(i => new WrappedNodeExpr(i)),
};
const res = compileInjector(meta);
return this.jitExpression(res.expression, angularCoreEnv, sourceMapUrl, res.statements);
Expand Down
1 change: 1 addition & 0 deletions packages/compiler/src/render3/r3_identifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ export class Identifiers {
};

static defineNgModule: o.ExternalReference = {name: 'ɵdefineNgModule', moduleName: CORE};
static setNgModuleScope: o.ExternalReference = {name: 'ɵsetNgModuleScope', moduleName: CORE};

static PipeDefWithMeta: o.ExternalReference = {name: 'ɵPipeDefWithMeta', moduleName: CORE};

Expand Down
Loading