Merge pull request #204 from ckeditor/t/203

Fix: ViewCollection#destroy should wait for all ViewCollection#add promises to resolve to avoid errors. Closes #203.

Author: oleq

src/view.js

@@ -269,15 +269,14 @@ export default class View {
 	 *		}
 	 *
 	 * @param {module:ui/view~View|Iterable.<module:ui/view~View>} 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;
+			} );
 	}
 
 	/**

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;

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 );
+				} );
+			} );
+		} );
 	} );
 } );
 

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()', () => {