Browse Source

Merge pull request #149 from ckeditor/t/148

t/148: Log when user attempts to extend a Template instance which has alread…
Piotrek Koszuliński 8 years ago
parent
commit
f891568aa7
2 changed files with 71 additions and 5 deletions
  1. 27 1
      packages/ckeditor5-ui/src/template.js
  2. 44 4
      packages/ckeditor5-ui/tests/template.js

+ 27 - 1
packages/ckeditor5-ui/src/template.js

@@ -17,6 +17,7 @@ import View from './view';
 import ViewCollection from './viewcollection';
 import cloneDeepWith from '@ckeditor/ckeditor5-utils/src/lib/lodash/cloneDeepWith';
 import isObject from '@ckeditor/ckeditor5-utils/src/lib/lodash/isObject';
+import log from '@ckeditor/ckeditor5-utils/src/log';
 
 const xhtmlNs = 'http://www.w3.org/1999/xhtml';
 
@@ -56,6 +57,16 @@ export default class Template {
 		Object.assign( this, normalize( clone( def ) ) );
 
 		/**
+		 * Indicates whether this particular Template instance has been
+		 * {@link #render rendered}.
+		 *
+		 * @readonly
+		 * @protected
+		 * @member {Boolean}
+		 */
+		this._isRendered = false;
+
+		/**
 		 * Tag of this template, i.e. `div`, indicating that the instance will render
 		 * to an HTMLElement.
 		 *
@@ -101,7 +112,11 @@ export default class Template {
 	 * @returns {HTMLElement|Text}
 	 */
 	render() {
-		return this._renderNode( undefined, true );
+		const node = this._renderNode( undefined, true );
+
+		this._isRendered = true;
+
+		return node;
 	}
 
 	/**
@@ -232,6 +247,17 @@ export default class Template {
 	 * @param {module:ui/template~TemplateDefinition} def An extension to existing an template instance.
 	 */
 	static extend( template, def ) {
+		if ( template._isRendered ) {
+			/**
+			 * Extending a template after rendering may not work as expected. To make sure
+			 * the {@link #extend extending} works for the rendered element, perform it
+			 * before {@link #render} is called.
+			 *
+			 * @error template-extend-render
+			 */
+			log.warn( 'template-extend-render: Attempting to extend a template which has already been rendered.' );
+		}
+
 		extendTemplate( template, normalize( clone( def ) ) );
 	}
 

+ 44 - 4
packages/ckeditor5-ui/tests/template.js

@@ -16,6 +16,7 @@ import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
 import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin';
 import Collection from '@ckeditor/ckeditor5-utils/src/collection';
 import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml';
+import log from '@ckeditor/ckeditor5-utils/src/log';
 
 testUtils.createSinonSandbox();
 
@@ -23,6 +24,10 @@ let el, text;
 
 describe( 'Template', () => {
 	describe( 'constructor()', () => {
+		it( 'sets #_isRendered property', () => {
+			expect( new Template( { tag: 'p' } )._isRendered ).to.be.false;
+		} );
+
 		it( 'accepts and normalizes the definition', () => {
 			const bind = Template.bind( new Model( {} ), Object.create( DomEmitterMixin ) );
 			const tpl = new Template( {
@@ -119,7 +124,7 @@ describe( 'Template', () => {
 		} );
 	} );
 
-	describe( 'render', () => {
+	describe( 'render()', () => {
 		it( 'throws when the template definition is wrong', () => {
 			expect( () => {
 				new Template( {} ).render();
@@ -133,6 +138,16 @@ describe( 'Template', () => {
 			} ).to.throw( CKEditorError, /ui-template-wrong-syntax/ );
 		} );
 
+		it( 'sets #_isRendered true', () => {
+			const tpl = new Template( { tag: 'p' } );
+
+			expect( tpl._isRendered ).to.be.false;
+
+			tpl.render();
+
+			expect( tpl._isRendered ).to.be.true;
+		} );
+
 		describe( 'DOM Node', () => {
 			it( 'creates HTMLElement', () => {
 				const el = new Template( {
@@ -609,7 +624,7 @@ describe( 'Template', () => {
 		} );
 	} );
 
-	describe( 'apply', () => {
+	describe( 'apply()', () => {
 		let observable, domEmitter, bind;
 
 		beforeEach( () => {
@@ -857,7 +872,7 @@ describe( 'Template', () => {
 		} );
 	} );
 
-	describe( 'bind', () => {
+	describe( 'bind()', () => {
 		it( 'returns object', () => {
 			expect( Template.bind() ).to.be.an( 'object' );
 		} );
@@ -1586,7 +1601,7 @@ describe( 'Template', () => {
 		} );
 	} );
 
-	describe( 'extend', () => {
+	describe( 'extend()', () => {
 		let observable, emitter, bind;
 
 		beforeEach( () => {
@@ -1621,6 +1636,31 @@ describe( 'Template', () => {
 			expect( tpl.attributes.b[ 0 ] ).to.equal( 'bar' );
 		} );
 
+		it( 'logs a warning if an element has already been rendered', () => {
+			const spy = testUtils.sinon.spy( log, 'warn' );
+
+			const tpl = new Template( {
+				tag: 'p'
+			} );
+
+			Template.extend( tpl, {
+				attributes: {
+					class: 'foo'
+				}
+			} );
+
+			tpl.render();
+
+			Template.extend( tpl, {
+				attributes: {
+					class: 'bar'
+				}
+			} );
+
+			sinon.assert.calledOnce( spy );
+			sinon.assert.calledWithExactly( spy, sinon.match( /^template-extend-render/ ) );
+		} );
+
 		describe( 'attributes', () => {
 			it( 'extends existing - simple', () => {
 				extensionTest(