diff --git a/src/view.js b/src/view.js index d7a89697..59ec809d 100644 --- a/src/view.js +++ b/src/view.js @@ -269,15 +269,14 @@ export default class View { * } * * @param {module:ui/view~View|Iterable.} children Children views to be registered. + * @returns {Promise} */ addChildren( children ) { if ( !isIterable( children ) ) { children = [ children ]; } - for ( let child of children ) { - this._unboundChildren.add( child ); - } + return Promise.all( children.map( c => this._unboundChildren.add( c ) ) ); } /** @@ -314,15 +313,14 @@ export default class View { destroy() { this.stopListening(); - const promises = this._viewCollections.map( c => c.destroy() ); - - this._unboundChildren.clear(); - this._viewCollections.clear(); - - this.element = this.template = this.locale = this.t = - this._viewCollections = this._unboundChildren = null; + return Promise.all( this._viewCollections.map( c => c.destroy() ) ) + .then( () => { + this._unboundChildren.clear(); + this._viewCollections.clear(); - return Promise.all( promises ); + this.element = this.template = this.locale = this.t = + this._viewCollections = this._unboundChildren = null; + } ); } /** diff --git a/src/viewcollection.js b/src/viewcollection.js index a51e04fd..5d05ede9 100644 --- a/src/viewcollection.js +++ b/src/viewcollection.js @@ -69,6 +69,16 @@ export default class ViewCollection extends Collection { * @member {HTMLElement} */ this._parentElement = null; + + /** + * A set containing promises created by {@link #add}. If the {@link #destroy} is called + * before the child views' {@link module:ui/view~View#init} are completed, + * {@link #destroy} will wait until all the promises are resolved. + * + * @private + * @member {Set} + */ + this._addPromises = new Set(); } /** @@ -99,13 +109,13 @@ export default class ViewCollection extends Collection { * @returns {Promise} A Promise resolved when the destruction process is finished. */ destroy() { - let promises = []; - - for ( let view of this ) { - promises.push( view.destroy() ); - } - - return Promise.all( promises ); + // Wait for all #add() promises to resolve before destroying the children. + // https://github.com/ckeditor/ckeditor5-ui/issues/203 + return Promise.all( this._addPromises ) + // Then begin the process of destroying the children. + .then( () => { + return Promise.all( this.map( v => v.destroy() ) ); + } ); } /** @@ -123,9 +133,15 @@ export default class ViewCollection extends Collection { let promise = Promise.resolve(); if ( this.ready && !view.ready ) { - promise = promise.then( () => { - return view.init(); - } ); + promise = promise + .then( () => view.init() ) + // The view is ready. There's no point in storing the promise any longer. + .then( () => this._addPromises.delete( promise ) ); + + // Store the promise so it can be respected (and resolved) before #destroy() + // starts destroying the child view. + // https://github.com/ckeditor/ckeditor5-ui/issues/203 + this._addPromises.add( promise ); } return promise; diff --git a/tests/view.js b/tests/view.js index 624c9429..02e53937 100644 --- a/tests/view.js +++ b/tests/view.js @@ -87,21 +87,47 @@ describe( 'View', () => { setTestViewInstance(); } ); + it( 'should return a promise', () => { + const spy = sinon.spy(); + const child = { + init: () => { + return new Promise( resolve => { + setTimeout( () => resolve(), 100 ); + } ) + .then( () => spy() ); + } + }; + + return view.init() + .then( () => { + const returned = view.addChildren( child ); + expect( returned ).to.be.instanceof( Promise ); + + return returned.then( () => { + sinon.assert.calledOnce( spy ); + } ); + } ); + } ); + it( 'should add a single view to #_unboundChildren', () => { expect( view._unboundChildren ).to.have.length( 0 ); const child = {}; - view.addChildren( child ); - expect( view._unboundChildren ).to.have.length( 1 ); - expect( view._unboundChildren.get( 0 ) ).to.equal( child ); + return view.addChildren( child ) + .then( () => { + expect( view._unboundChildren ).to.have.length( 1 ); + expect( view._unboundChildren.get( 0 ) ).to.equal( child ); + } ); } ); it( 'should support iterables', () => { expect( view._unboundChildren ).to.have.length( 0 ); - view.addChildren( [ {}, {}, {} ] ); - expect( view._unboundChildren ).to.have.length( 3 ); + return view.addChildren( [ {}, {}, {} ] ) + .then( () => { + expect( view._unboundChildren ).to.have.length( 3 ); + } ); } ); } ); @@ -254,12 +280,14 @@ describe( 'View', () => { it( 'clears #_unboundChildren', () => { const cached = view._unboundChildren; - view.addChildren( [ new View(), new View() ] ); - expect( cached ).to.have.length.above( 2 ); + return view.addChildren( [ new View(), new View() ] ) + .then( () => { + expect( cached ).to.have.length.above( 2 ); - return view.destroy().then( () => { - expect( cached ).to.have.length( 0 ); - } ); + return view.destroy().then( () => { + expect( cached ).to.have.length( 0 ); + } ); + } ); } ); it( 'clears #_viewCollections', () => { @@ -304,6 +332,44 @@ describe( 'View', () => { view.destroy(); } ).to.not.throw(); } ); + + // https://github.com/ckeditor/ckeditor5-ui/issues/203 + it( 'waits for all #addChildren promises to resolve', () => { + const spyA = sinon.spy(); + const spyB = sinon.spy(); + + class DelayedInitView extends View { + constructor( delay, spy ) { + super(); + + this.delay = delay; + this.spy = spy; + } + + init() { + return new Promise( resolve => { + setTimeout( () => resolve(), this.delay ); + } ) + .then( () => super.init() ) + .then( () => { + this.spy(); + } ); + } + } + + const viewA = new DelayedInitView( 200, spyA ); + const viewB = new DelayedInitView( 100, spyB ); + + return view.init().then( () => { + view.addChildren( [ viewA, viewB ] ); + + return view.destroy().then( () => { + expect( viewA.ready ).to.be.true; + expect( viewB.ready ).to.be.true; + sinon.assert.callOrder( spyB, spyA ); + } ); + } ); + } ); } ); } ); diff --git a/tests/viewcollection.js b/tests/viewcollection.js index 8f0823fc..ce6ac3bf 100644 --- a/tests/viewcollection.js +++ b/tests/viewcollection.js @@ -234,6 +234,46 @@ describe( 'ViewCollection', () => { sinon.assert.callOrder( spyA, spyB ); } ); } ); + + // https://github.com/ckeditor/ckeditor5-ui/issues/203 + it( 'waits for all #add promises to resolve', () => { + const spyA = sinon.spy(); + const spyB = sinon.spy(); + + class DelayedInitView extends View { + constructor( delay, spy ) { + super(); + + this.delay = delay; + this.spy = spy; + } + + init() { + return new Promise( resolve => { + setTimeout( () => resolve(), this.delay ); + } ) + .then( () => super.init() ) + .then( () => { + this.spy(); + } ); + } + } + + const viewA = new DelayedInitView( 200, spyA ); + const viewB = new DelayedInitView( 100, spyB ); + + return collection.init().then( () => { + collection.add( viewA ); + collection.add( viewB ); + } ) + .then( () => { + return collection.destroy().then( () => { + expect( viewA.ready ).to.be.true; + expect( viewB.ready ).to.be.true; + sinon.assert.callOrder( spyB, spyA ); + } ); + } ); + } ); } ); describe( 'add()', () => {