From f97b91adcf064959f2d06df44ab81531d6e194d0 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 13 Apr 2017 13:58:21 +0200 Subject: [PATCH 1/5] Fix: ViewCollection#destroy should wait for all ViewCollection#add promises to resolve to avoid errors. Closes #203. --- src/viewcollection.js | 41 +++++++++++++++++++++++++++++++++-------- tests/viewcollection.js | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/viewcollection.js b/src/viewcollection.js index a51e04fd..39cc8e47 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,19 @@ export default class ViewCollection extends Collection { * @returns {Promise} A Promise resolved when the destruction process is finished. */ destroy() { - let 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( () => { + let destroyPromises = []; - for ( let view of this ) { - promises.push( view.destroy() ); - } + for ( let view of this ) { + destroyPromises.push( view.destroy() ); + } - return Promise.all( promises ); + return Promise.all( destroyPromises ); + } ); } /** @@ -123,9 +139,18 @@ 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() ) + .then( () => { + // The view is ready. There's no point in storing the promise + // any longer. + 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/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()', () => { From 829db676701cdff577696136c7148c3ee590e072 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 13 Apr 2017 14:33:48 +0200 Subject: [PATCH 2/5] Code refactoring: Simplified ViewCollection#destroy. --- src/viewcollection.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/viewcollection.js b/src/viewcollection.js index 39cc8e47..eb378815 100644 --- a/src/viewcollection.js +++ b/src/viewcollection.js @@ -114,13 +114,7 @@ export default class ViewCollection extends Collection { return Promise.all( this._addPromises ) // Then begin the process of destroying the children. .then( () => { - let destroyPromises = []; - - for ( let view of this ) { - destroyPromises.push( view.destroy() ); - } - - return Promise.all( destroyPromises ); + return Promise.all( Array.from( this, view => view.destroy() ) ); } ); } From f1f5226ffa7eecff135750b2705c6f38ca872987 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 13 Apr 2017 16:00:54 +0200 Subject: [PATCH 3/5] Code refactoring in ViewCollection class. --- src/viewcollection.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/viewcollection.js b/src/viewcollection.js index eb378815..ec430b7f 100644 --- a/src/viewcollection.js +++ b/src/viewcollection.js @@ -135,11 +135,8 @@ export default class ViewCollection extends Collection { if ( this.ready && !view.ready ) { promise = promise .then( () => view.init() ) - .then( () => { - // The view is ready. There's no point in storing the promise - // any longer. - this._addPromises.delete( promise ); - } ); + // 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. From 16ef46d0b24d13962c7bdcc53d9300a9b450f7be Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 13 Apr 2017 16:01:27 +0200 Subject: [PATCH 4/5] Made View#addChildren return Promise. --- src/view.js | 22 +++++++------ tests/view.js | 86 +++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 89 insertions(+), 19 deletions(-) diff --git a/src/view.js b/src/view.js index d7a89697..ed16a694 100644 --- a/src/view.js +++ b/src/view.js @@ -269,15 +269,20 @@ 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 ]; } + const promises = []; + for ( let child of children ) { - this._unboundChildren.add( child ); + promises.push( this._unboundChildren.add( child ) ); } + + return Promise.all( promises ); } /** @@ -314,15 +319,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( Array.from( this._viewCollections, 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/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 ); + } ); + } ); + } ); } ); } ); From f06fbcd358f15b3f98a3d3160be7b1136e8b1e7e Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 19 Apr 2017 12:11:49 +0200 Subject: [PATCH 5/5] Code refactoring: simplified Promise.all calls in View and ViewCollection classes. --- src/view.js | 10 ++-------- src/viewcollection.js | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/view.js b/src/view.js index ed16a694..59ec809d 100644 --- a/src/view.js +++ b/src/view.js @@ -276,13 +276,7 @@ export default class View { children = [ children ]; } - const promises = []; - - for ( let child of children ) { - promises.push( this._unboundChildren.add( child ) ); - } - - return Promise.all( promises ); + return Promise.all( children.map( c => this._unboundChildren.add( c ) ) ); } /** @@ -319,7 +313,7 @@ export default class View { destroy() { this.stopListening(); - return Promise.all( Array.from( this._viewCollections, c => c.destroy() ) ) + return Promise.all( this._viewCollections.map( c => c.destroy() ) ) .then( () => { this._unboundChildren.clear(); this._viewCollections.clear(); diff --git a/src/viewcollection.js b/src/viewcollection.js index ec430b7f..5d05ede9 100644 --- a/src/viewcollection.js +++ b/src/viewcollection.js @@ -114,7 +114,7 @@ export default class ViewCollection extends Collection { return Promise.all( this._addPromises ) // Then begin the process of destroying the children. .then( () => { - return Promise.all( Array.from( this, view => view.destroy() ) ); + return Promise.all( this.map( v => v.destroy() ) ); } ); }