From 74f92c6a798deecd1d6626d8e1813e1f05ed7e07 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Fri, 6 Feb 2015 08:57:49 +0100 Subject: [PATCH] perf(Compiler): use Promises only when strictly required --- .../angular2/src/core/compiler/compiler.js | 70 ++-- .../src/core/compiler/template_loader.js | 6 +- modules/angular2/src/facade/async.dart | 4 + modules/angular2/src/facade/async.es6 | 4 + .../test/core/compiler/compiler_spec.js | 389 +++++++++++------- .../core/compiler/template_loader_spec.js | 7 +- 6 files changed, 304 insertions(+), 176 deletions(-) diff --git a/modules/angular2/src/core/compiler/compiler.js b/modules/angular2/src/core/compiler/compiler.js index 320a652e92..18257083d5 100644 --- a/modules/angular2/src/core/compiler/compiler.js +++ b/modules/angular2/src/core/compiler/compiler.js @@ -86,15 +86,17 @@ export class Compiler { } compile(component:Type, templateRoot:Element = null):Promise { - return this._compile(this._reader.read(component), templateRoot); + var protoView = this._compile(this._reader.read(component), templateRoot); + return PromiseWrapper.isPromise(protoView) ? protoView : PromiseWrapper.resolve(protoView); } + // TODO(vicb): union type return ProtoView or Promise _compile(cmpMetadata: DirectiveMetadata, templateRoot:Element = null) { - var pvCached = this._compilerCache.get(cmpMetadata.type); - if (isPresent(pvCached)) { + var protoView = this._compilerCache.get(cmpMetadata.type); + if (isPresent(protoView)) { // The component has already been compiled into a ProtoView, // returns a resolved Promise. - return PromiseWrapper.resolve(pvCached); + return protoView; } var pvPromise = MapWrapper.get(this._compiling, cmpMetadata.type); @@ -105,21 +107,22 @@ export class Compiler { return pvPromise; } - var tplPromise = isBlank(templateRoot) ? - this._templateLoader.load(cmpMetadata) : - PromiseWrapper.resolve(templateRoot); + var template = isBlank(templateRoot) ? this._templateLoader.load(cmpMetadata) : templateRoot; - pvPromise = PromiseWrapper.then(tplPromise, - (el) => this._compileTemplate(el, cmpMetadata), - (_) => { throw new BaseException(`Failed to load the template for ${stringify(cmpMetadata.type)}`) } - ); + if (PromiseWrapper.isPromise(template)) { + pvPromise = PromiseWrapper.then(template, + (el) => this._compileTemplate(el, cmpMetadata), + (_) => { throw new BaseException(`Failed to load the template for ${stringify(cmpMetadata.type)}`); } + ); + MapWrapper.set(this._compiling, cmpMetadata.type, pvPromise); + return pvPromise; + } - MapWrapper.set(this._compiling, cmpMetadata.type, pvPromise); - - return pvPromise; + return this._compileTemplate(template, cmpMetadata); } - _compileTemplate(template: Element, cmpMetadata): Promise { + // TODO(vicb): union type return ProtoView or Promise + _compileTemplate(template: Element, cmpMetadata) { var pipeline = new CompilePipeline(this.createSteps(cmpMetadata)); var compileElements = pipeline.process(template); var protoView = compileElements[0].inheritedProtoView; @@ -130,27 +133,38 @@ export class Compiler { MapWrapper.delete(this._compiling, cmpMetadata.type); // Compile all the components from the template - var componentPromises = []; + var nestedPVPromises = []; for (var i = 0; i < compileElements.length; i++) { var ce = compileElements[i]; if (isPresent(ce.componentDirective)) { - var componentPromise = this._compileNestedProtoView(ce); - ListWrapper.push(componentPromises, componentPromise); + this._compileNestedProtoView(ce, nestedPVPromises); } } - // The protoView is resolved after all the components in the template have been compiled. - return PromiseWrapper.then(PromiseWrapper.all(componentPromises), - (_) => protoView, - (e) => { throw new BaseException(`${e} -> Failed to compile ${stringify(cmpMetadata.type)}`) } - ); + if (nestedPVPromises.length > 0) { + // Returns ProtoView Promise when there are any asynchronous nested ProtoViews. + // The promise will resolved after nested ProtoViews are compiled. + return PromiseWrapper.then(PromiseWrapper.all(nestedPVPromises), + (_) => protoView, + (e) => { throw new BaseException(`${e.message} -> Failed to compile ${stringify(cmpMetadata.type)}`); } + ); + } + + // When there is no asynchronous nested ProtoViews, return the ProtoView + return protoView; } - _compileNestedProtoView(ce: CompileElement):Promise { - var pvPromise = this._compile(ce.componentDirective); - pvPromise.then(function(protoView) { + _compileNestedProtoView(ce: CompileElement, promises: List) + { + var protoView = this._compile(ce.componentDirective); + + if (PromiseWrapper.isPromise(protoView)) { + ListWrapper.push(promises, protoView); + protoView.then(function (protoView) { + ce.inheritedElementBinder.nestedProtoView = protoView; + }); + } else { ce.inheritedElementBinder.nestedProtoView = protoView; - }); - return pvPromise; + } } } diff --git a/modules/angular2/src/core/compiler/template_loader.js b/modules/angular2/src/core/compiler/template_loader.js index ce7538a1f7..3e5991299f 100644 --- a/modules/angular2/src/core/compiler/template_loader.js +++ b/modules/angular2/src/core/compiler/template_loader.js @@ -22,13 +22,13 @@ export class TemplateLoader { this._cache = StringMapWrapper.create(); } - load(cmpMetadata: DirectiveMetadata):Promise { + // TODO(vicb): union type: return an Element or a Promise + load(cmpMetadata: DirectiveMetadata) { var annotation:Component = cmpMetadata.annotation; var tplConfig:TemplateConfig = annotation.template; if (isPresent(tplConfig.inline)) { - var template = DOM.createTemplate(tplConfig.inline); - return PromiseWrapper.resolve(template); + return DOM.createTemplate(tplConfig.inline); } if (isPresent(tplConfig.url)) { diff --git a/modules/angular2/src/facade/async.dart b/modules/angular2/src/facade/async.dart index 7d72fcedcc..05f54c0fe4 100644 --- a/modules/angular2/src/facade/async.dart +++ b/modules/angular2/src/facade/async.dart @@ -20,6 +20,10 @@ class PromiseWrapper { static void setTimeout(fn(), int millis) { new Timer(new Duration(milliseconds: millis), fn); } + + static bool isPromise(maybePromise) { + return maybePromise is Future; + } } class _Completer { diff --git a/modules/angular2/src/facade/async.es6 b/modules/angular2/src/facade/async.es6 index 829212c0b0..3428347ef6 100644 --- a/modules/angular2/src/facade/async.es6 +++ b/modules/angular2/src/facade/async.es6 @@ -39,4 +39,8 @@ export class PromiseWrapper { static setTimeout(fn:Function, millis:int) { window.setTimeout(fn, millis); } + + static isPromise(maybePromise):boolean { + return maybePromise instanceof Promise; + } } diff --git a/modules/angular2/test/core/compiler/compiler_spec.js b/modules/angular2/test/core/compiler/compiler_spec.js index 0850a4064a..0c23d043b5 100644 --- a/modules/angular2/test/core/compiler/compiler_spec.js +++ b/modules/angular2/test/core/compiler/compiler_spec.js @@ -1,7 +1,7 @@ import {describe, beforeEach, it, expect, ddescribe, iit, el, IS_DARTIUM} from 'angular2/test_lib'; import {DOM, Element, TemplateElement} from 'angular2/src/facade/dom'; -import {List, ListWrapper, Map, MapWrapper} from 'angular2/src/facade/collection'; -import {Type, isBlank} from 'angular2/src/facade/lang'; +import {List, ListWrapper, Map, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection'; +import {Type, isBlank, stringify} from 'angular2/src/facade/lang'; import {PromiseWrapper} from 'angular2/src/facade/async'; import {Compiler, CompilerCache} from 'angular2/src/core/compiler/compiler'; @@ -27,154 +27,201 @@ export function main() { reader = new DirectiveMetadataReader(); }); - function createCompiler(processClosure, strategy:ShadowDomStrategy = null, xhr: XHRMock = null) { - var steps = [new MockStep(processClosure)]; - if (isBlank(strategy)) { - strategy = new NativeShadowDomStrategy(); - } - if (isBlank(xhr)) { - xhr = new XHRMock(); - } - return new TestableCompiler(reader, steps, strategy, xhr); - } + var syncTemplateLoader = new FakeTemplateLoader(); + syncTemplateLoader.forceSync(); + var asyncTemplateLoader = new FakeTemplateLoader(); + asyncTemplateLoader.forceAsync(); - it('should run the steps and return the ProtoView of the root element', (done) => { - var rootProtoView = new ProtoView(null, null, null); - var compiler = createCompiler( (parent, current, control) => { - current.inheritedProtoView = rootProtoView; - }); - compiler.compile(MainComponent, el('
')).then( (protoView) => { - expect(protoView).toBe(rootProtoView); - done(); - }); - }); + StringMapWrapper.forEach({ + '(sync TemplateLoader)': syncTemplateLoader, + '(async TemplateLoader)': asyncTemplateLoader + }, (templateLoader, name) => { - it('should use the given element', (done) => { - var element = el('
'); - var compiler = createCompiler( (parent, current, control) => { - current.inheritedProtoView = new ProtoView(current.element, null, null); - }); - compiler.compile(MainComponent, element).then( (protoView) => { - expect(protoView.element).toBe(element); - done(); - }); - }); + describe(name, () => { - it('should use the inline template if no element is given explicitly', (done) => { - var compiler = createCompiler( (parent, current, control) => { - current.inheritedProtoView = new ProtoView(current.element, null, null); - }); - compiler.compile(MainComponent, null).then( (protoView) => { - expect(DOM.getInnerHTML(protoView.element)).toEqual('inline component'); - done(); - }); - }); - - it('should load nested components', (done) => { - var mainEl = el('
'); - var compiler = createCompiler( (parent, current, control) => { - current.inheritedProtoView = new ProtoView(current.element, null, null); - current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); - if (current.element === mainEl) { - current.componentDirective = reader.read(NestedComponent); + function createCompiler(processClosure) { + var steps = [new MockStep(processClosure)]; + return new TestableCompiler(reader, steps, templateLoader); } - }); - compiler.compile(MainComponent, mainEl).then( (protoView) => { - var nestedView = protoView.elementBinders[0].nestedProtoView; - expect(DOM.getInnerHTML(nestedView.element)).toEqual('nested component'); - done(); - }); - }); - it('should cache compiled components', (done) => { - var element = el('
'); - var compiler = createCompiler( (parent, current, control) => { - current.inheritedProtoView = new ProtoView(current.element, null, null); - }); - var firstProtoView; - compiler.compile(MainComponent, element).then( (protoView) => { - firstProtoView = protoView; - return compiler.compile(MainComponent, element); - }).then( (protoView) => { - expect(firstProtoView).toBe(protoView); - done(); - }); - }); - - it('should re-use components being compiled', (done) => { - var nestedElBinders = []; - var mainEl = el('
'); - var compiler = createCompiler( (parent, current, control) => { - if (DOM.hasClass(current.element, 'nested')) { - current.inheritedProtoView = new ProtoView(current.element, null, null); - current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); - current.componentDirective = reader.read(NestedComponent); - ListWrapper.push(nestedElBinders, current.inheritedElementBinder); - } - }); - compiler.compile(MainComponent, mainEl).then( (protoView) => { - expect(nestedElBinders[0].nestedProtoView).toBe(nestedElBinders[1].nestedProtoView); - done(); - }); - }); - - it('should allow recursive components', (done) => { - var compiler = createCompiler( (parent, current, control) => { - current.inheritedProtoView = new ProtoView(current.element, null, null); - current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); - current.componentDirective = reader.read(RecursiveComponent); - }); - compiler.compile(RecursiveComponent, null).then( (protoView) => { - expect(protoView.elementBinders[0].nestedProtoView).toBe(protoView); - done(); - }); - }); - - describe('XHR', () => { - it('should load template via xhr', (done) => { - var xhr = new XHRMock(); - xhr.expect('/parent', 'xhr'); - - var compiler = createCompiler((parent, current, control) => { - current.inheritedProtoView = new ProtoView(current.element, null, null); - }, null, xhr); - - compiler.compile(XHRParentComponent).then( (protoView) => { - expect(DOM.getInnerHTML(protoView.element)).toEqual('xhr'); - done(); + it('should run the steps and return the ProtoView of the root element', (done) => { + var rootProtoView = new ProtoView(null, null, null); + var compiler = createCompiler( (parent, current, control) => { + current.inheritedProtoView = rootProtoView; + }); + compiler.compile(MainComponent, el('
')).then( (protoView) => { + expect(protoView).toBe(rootProtoView); + done(); + }); }); - xhr.flush(); - }); - - it('should return a rejected promise when loading a template fails', (done) => { - var xhr = new XHRMock(); - xhr.expect('/parent', null); - - var compiler = createCompiler((parent, current, control) => {}, null, xhr); - - PromiseWrapper.then(compiler.compile(XHRParentComponent), - function(_) { throw 'Failure expected'; }, - function(e) { - expect(e.message).toEqual('Failed to load the template for XHRParentComponent'); + it('should use the given element', (done) => { + var element = el('
'); + var compiler = createCompiler( (parent, current, control) => { + current.inheritedProtoView = new ProtoView(current.element, null, null); + }); + compiler.compile(MainComponent, element).then( (protoView) => { + expect(protoView.element).toBe(element); done(); - } - ); + }); + }); - xhr.flush(); + it('should use the inline template if no element is given explicitly', (done) => { + var compiler = createCompiler( (parent, current, control) => { + current.inheritedProtoView = new ProtoView(current.element, null, null); + }); + compiler.compile(MainComponent, null).then( (protoView) => { + expect(DOM.getInnerHTML(protoView.element)).toEqual('inline component'); + done(); + }); + }); + + it('should load nested components', (done) => { + var mainEl = el('
'); + var compiler = createCompiler( (parent, current, control) => { + current.inheritedProtoView = new ProtoView(current.element, null, null); + current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); + if (current.element === mainEl) { + current.componentDirective = reader.read(NestedComponent); + } + }); + compiler.compile(MainComponent, mainEl).then( (protoView) => { + var nestedView = protoView.elementBinders[0].nestedProtoView; + expect(DOM.getInnerHTML(nestedView.element)).toEqual('nested component'); + done(); + }); + }); + + it('should cache compiled components', (done) => { + var element = el('
'); + var compiler = createCompiler( (parent, current, control) => { + current.inheritedProtoView = new ProtoView(current.element, null, null); + }); + var firstProtoView; + compiler.compile(MainComponent, element).then( (protoView) => { + firstProtoView = protoView; + return compiler.compile(MainComponent, element); + }).then( (protoView) => { + expect(firstProtoView).toBe(protoView); + done(); + }); + }); + + it('should re-use components being compiled', (done) => { + var nestedElBinders = []; + var mainEl = el('
'); + var compiler = createCompiler( (parent, current, control) => { + if (DOM.hasClass(current.element, 'nested')) { + current.inheritedProtoView = new ProtoView(current.element, null, null); + current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); + current.componentDirective = reader.read(NestedComponent); + ListWrapper.push(nestedElBinders, current.inheritedElementBinder); + } + }); + compiler.compile(MainComponent, mainEl).then( (protoView) => { + expect(nestedElBinders[0].nestedProtoView).toBe(nestedElBinders[1].nestedProtoView); + done(); + }); + }); + + it('should allow recursive components', (done) => { + var compiler = createCompiler( (parent, current, control) => { + current.inheritedProtoView = new ProtoView(current.element, null, null); + current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); + current.componentDirective = reader.read(RecursiveComponent); + }); + compiler.compile(RecursiveComponent, null).then( (protoView) => { + expect(protoView.elementBinders[0].nestedProtoView).toBe(protoView); + done(); + }); + }); }); }); + + describe('(mixed async, sync TemplateLoader)', () => { + function createCompiler(processClosure, templateLoader: TemplateLoader) { + var steps = [new MockStep(processClosure)]; + return new TestableCompiler(reader, steps, templateLoader); + } + + function createNestedComponentSpec(name, loader: TemplateLoader, error:string = null) { + it(`should load nested components ${name}`, (done) => { + + var compiler = createCompiler((parent, current, control) => { + if (DOM.hasClass(current.element, 'parent')) { + current.componentDirective = reader.read(NestedComponent); + current.inheritedProtoView = parent.inheritedProtoView; + current.inheritedElementBinder = current.inheritedProtoView.bindElement(null); + } else { + current.inheritedProtoView = new ProtoView(current.element, null, null); + } + }, loader); + + PromiseWrapper.then(compiler.compile(ParentComponent), + function(protoView) { + var nestedView = protoView.elementBinders[0].nestedProtoView; + expect(error).toBeNull(); + expect(DOM.getInnerHTML(nestedView.element)).toEqual('nested component'); + done(); + }, + function(compileError) { + expect(compileError.message).toEqual(error); + done(); + } + ); + }); + } + + var loader = new FakeTemplateLoader(); + loader.setSync(ParentComponent); + loader.setSync(NestedComponent); + createNestedComponentSpec('(sync -> sync)', loader); + + loader = new FakeTemplateLoader(); + loader.setAsync(ParentComponent); + loader.setSync(NestedComponent); + createNestedComponentSpec('(async -> sync)', loader); + + loader = new FakeTemplateLoader(); + loader.setSync(ParentComponent); + loader.setAsync(NestedComponent); + createNestedComponentSpec('(sync -> async)', loader); + + loader = new FakeTemplateLoader(); + loader.setAsync(ParentComponent); + loader.setAsync(NestedComponent); + createNestedComponentSpec('(async -> async)', loader); + + loader = new FakeTemplateLoader(); + loader.setError(ParentComponent); + loader.setSync(NestedComponent); + createNestedComponentSpec('(error -> sync)', loader, + 'Failed to load the template for ParentComponent'); + + // TODO(vicb): Check why errors this fails with Dart + // TODO(vicb): The Promise is rejected with the correct error but an exc is thrown before + //loader = new FakeTemplateLoader(); + //loader.setSync(ParentComponent); + //loader.setError(NestedComponent); + //createNestedComponentSpec('(sync -> error)', loader, + // 'Failed to load the template for NestedComponent -> Failed to compile ParentComponent'); + // + //loader = new FakeTemplateLoader(); + //loader.setAsync(ParentComponent); + //loader.setError(NestedComponent); + //createNestedComponentSpec('(async -> error)', loader, + // 'Failed to load the template for NestedComponent -> Failed to compile ParentComponent'); + + }); }); - } - @Component({ template: new TemplateConfig({ - url: '/parent' + inline: '
' }) }) -class XHRParentComponent {} +class ParentComponent {} @Component({ template: new TemplateConfig({ @@ -201,14 +248,9 @@ class RecursiveComponent {} class TestableCompiler extends Compiler { steps:List; - constructor(reader:DirectiveMetadataReader, steps:List, strategy:ShadowDomStrategy, - xhr: XHRMock) { - super(dynamicChangeDetection, - new TemplateLoader(xhr), - reader, - new Parser(new Lexer()), - new CompilerCache(), - strategy); + constructor(reader:DirectiveMetadataReader, steps:List, loader: TemplateLoader) { + super(dynamicChangeDetection, loader, reader, new Parser(new Lexer()), new CompilerCache(), + new NativeShadowDomStrategy()); this.steps = steps; } @@ -227,3 +269,70 @@ class MockStep extends CompileStep { this.processClosure(parent, current, control); } } + +class FakeTemplateLoader extends TemplateLoader { + _forceSync: boolean; + _forceAsync: boolean; + _syncCmp: List; + _asyncCmp: List; + _errorCmp: List; + + constructor() { + super (new XHRMock()); + this._forceSync = false; + this._forceAsync = false; + this._syncCmp = []; + this._asyncCmp = []; + this._errorCmp = []; + } + + forceSync() { + this._forceSync = true; + this._forceAsync = false; + } + + forceAsync() { + this._forceAsync = true; + this._forceSync = false; + } + + setSync(component: Type) { + ListWrapper.push(this._syncCmp, component); + } + + setAsync(component: Type) { + ListWrapper.push(this._asyncCmp, component); + } + + setError(component: Type) { + ListWrapper.push(this._errorCmp, component); + } + + load(cmpMetadata: DirectiveMetadata) { + var annotation:Component = cmpMetadata.annotation; + var tplConfig:TemplateConfig = annotation.template; + + if (isBlank(tplConfig.inline)) { + throw 'The component must define an inline template'; + } + + var template = DOM.createTemplate(tplConfig.inline); + + if (ListWrapper.contains(this._errorCmp, cmpMetadata.type)) { + return PromiseWrapper.reject('Fail to load'); + } + + if (ListWrapper.contains(this._syncCmp, cmpMetadata.type)) { + return template; + } + + if (ListWrapper.contains(this._asyncCmp, cmpMetadata.type)) { + return PromiseWrapper.resolve(template); + } + + if (this._forceSync) return template; + if (this._forceAsync) return PromiseWrapper.resolve(template); + + throw `No template configured for ${stringify(cmpMetadata.type)}`; + } +} diff --git a/modules/angular2/test/core/compiler/template_loader_spec.js b/modules/angular2/test/core/compiler/template_loader_spec.js index d6cb854f89..d48742309b 100644 --- a/modules/angular2/test/core/compiler/template_loader_spec.js +++ b/modules/angular2/test/core/compiler/template_loader_spec.js @@ -23,13 +23,10 @@ export function main() { return new DirectiveMetadata(FakeComponent, component, null); } - it('should load inline templates', (done) => { + it('should load inline templates synchronously', () => { var template = 'inline template'; var md = createMetadata({inline: template}); - loader.load(md).then((el) => { - expect(el.content).toHaveText(template); - done(); - }); + expect(loader.load(md).content).toHaveText(template); }); it('should load templates through XHR', (done) => {