8
0
Pārlūkot izejas kodu

Merge pull request #75 from ckeditor/t/74

Model.set() should throw when property already exists.
Piotr Jasiun 10 gadi atpakaļ
vecāks
revīzija
a765bd8f12

+ 24 - 1
packages/ckeditor5-utils/src/model.js

@@ -12,7 +12,7 @@
  * @mixins EventEmitter
  */
 
-CKEDITOR.define( [ 'emittermixin', 'utils' ], function( EmitterMixin, utils ) {
+CKEDITOR.define( [ 'emittermixin', 'ckeditorerror', 'utils' ], function( EmitterMixin, CKEditorError, utils ) {
 	class Model {
 		/**
 		 * Creates a new Model instance.
@@ -47,6 +47,10 @@ CKEDITOR.define( [ 'emittermixin', 'utils' ], function( EmitterMixin, utils ) {
 		 *
 		 * It accepts also a single object literal containing key/value pairs with attributes to be set.
 		 *
+		 * This method throws the {@link model-set-cannot-override} error if the model instance already
+		 * have a property with a given attribute name. This prevents from mistakenly overriding existing
+		 * properties and methods, but means that `foo.set( 'bar', 1 )` may be slightly slower than `foo.bar = 1`.
+		 *
 		 * @param {String} name The attributes name.
 		 * @param {*} value The attributes value.
 		 */
@@ -60,6 +64,25 @@ CKEDITOR.define( [ 'emittermixin', 'utils' ], function( EmitterMixin, utils ) {
 				return;
 			}
 
+			if ( ( name in this ) && !( name in this._attributes ) ) {
+				/**
+				 * Cannot override an existing property.
+				 *
+				 * This error is thrown when trying to {@link Model#set set} an attribute with
+				 * a name of an already existing property. For example:
+				 *
+				 *		var model = new Model();
+				 *		model.property = 1;
+				 *		model.set( 'property', 2 );		// throws
+				 *
+				 *		model.set( 'attr', 1 );
+				 *		model.set( 'attr', 2 );			// ok, because this is an existing attribute.
+				 *
+				 * @error model-set-cannot-override
+				 */
+				throw new CKEditorError( 'model-set-cannot-override: Cannot override an existing property.' );
+			}
+
 			Object.defineProperty( this, name, {
 				enumerable: true,
 				configurable: true,

+ 30 - 1
packages/ckeditor5-utils/tests/mvc/model/model.js

@@ -5,7 +5,7 @@
 
 'use strict';
 
-var modules = bender.amd.require( 'model', 'eventinfo' );
+var modules = bender.amd.require( 'model', 'eventinfo', 'ckeditorerror' );
 
 var Car, car;
 
@@ -134,6 +134,35 @@ describe( 'Model', function() {
 			sinon.assert.notCalled( spy );
 			sinon.assert.notCalled( spyColor );
 		} );
+
+		it( 'should throw when overriding already existing property', function() {
+			var CKEditorError = modules.ckeditorerror;
+
+			car.normalProperty = 1;
+
+			expect( () => {
+				car.set( 'normalProperty', 2 );
+			} ).to.throw( CKEditorError, /^model-set-cannot-override/ );
+
+			expect( car ).to.have.property( 'normalProperty', 1 );
+		} );
+
+		it( 'should throw when overriding already existing property (in the prototype)', function() {
+			var CKEditorError = modules.ckeditorerror;
+			var Model = modules.model;
+
+			class Car extends Model {
+				method() {}
+			}
+
+			car = new Car();
+
+			expect( () => {
+				car.set( 'method', 2 );
+			} ).to.throw( CKEditorError, /^model-set-cannot-override/ );
+
+			expect( car.method ).to.be.a( 'function' );
+		} );
 	} );
 
 	describe( 'extend', function() {