Merge pull request #258 from canjs/bg-warn-148

Warns when value property is assigned an object.
canjs · Aug 31, 2017 · f51e93a · f51e93a
2 parents 0c389d4 + 8b0fb11
commit f51e93a
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 31 deletions.
diff --git a/can-define.js b/can-define.js
@@ -237,6 +237,13 @@ define.property = function(objPrototype, prop, definition, dataInitializers, com
 
 	// Determine a function that will provide the initial property value.
 	if ((definition.value !== undefined || definition.Value !== undefined)) {
+		//!steal-remove-start
+
+		// If value is an object or array, give a warning 
+		if (definition.value !== null && typeof definition.value === 'object') {
+			dev.warn("can-define: The value for options is set to an object. This will be shared by all instances of the DefineMap. Use a function that returns the object instead.");
+		}
+		//!steal-remove-end
 		getInitialValue = Observation.ignore(make.get.defaultValue(prop, definition, typeConvert, eventsSetter));
 	}
 
@@ -855,40 +862,40 @@ define.makeSimpleGetterSetter = function(prop){
 };
 
 define.Iterator = function(obj){
-  this.obj = obj;
-  this.definitions = Object.keys(obj._define.definitions);
-  this.instanceDefinitions = obj._instanceDefinitions ?
-    Object.keys(obj._instanceDefinitions) :
-    Object.keys(obj);
-  this.hasGet = typeof obj.get === "function";
+	this.obj = obj;
+	this.definitions = Object.keys(obj._define.definitions);
+	this.instanceDefinitions = obj._instanceDefinitions ?
+		Object.keys(obj._instanceDefinitions) :
+		Object.keys(obj);
+	this.hasGet = typeof obj.get === "function";
 };
 
 define.Iterator.prototype.next = function(){
-  var key;
-  if(this.definitions.length) {
-    key = this.definitions.shift();
-
-    // Getters should not be enumerable
-    var def = this.obj._define.definitions[key];
-    if(def.get) {
-      return this.next();
-    }
-  } else if(this.instanceDefinitions.length) {
-    key = this.instanceDefinitions.shift();
-  } else {
-    return {
-      value: undefined,
-      done: true
-    };
-  }
-
-  return {
-    value: [
-      key,
-      this.hasGet ? this.obj.get(key) : this.obj[key]
-    ],
-    done: false
-  };
+	var key;
+	if(this.definitions.length) {
+		key = this.definitions.shift();
+
+		// Getters should not be enumerable
+		var def = this.obj._define.definitions[key];
+		if(def.get) {
+			return this.next();
+		}
+	} else if(this.instanceDefinitions.length) {
+		key = this.instanceDefinitions.shift();
+	} else {
+		return {
+			value: undefined,
+			done: true
+		};
+	}
+
+	return {
+		value: [
+			key,
+			this.hasGet ? this.obj.get(key) : this.obj[key]
+		],
+		done: false
+	};
 };
 
 isDefineType = function(func){

diff --git a/map/map-test.js b/map/map-test.js
@@ -8,6 +8,7 @@ var compute = require("can-compute");
 var assign = require("can-util/js/assign/assign");
 var canReflect = require("can-reflect");
 var isPlainObject = require("can-util/js/is-plain-object/is-plain-object");
+var canDev = require("can-util/js/dev/dev");
 
 var sealWorks = (function() {
 	try {
@@ -836,3 +837,40 @@ QUnit.test("non-Object constructor", function() {
 	QUnit.ok(!isPlainObject(new DefineMap()), "instance of DefineMap is not a plain object");
 	QUnit.ok(!isPlainObject(new Constructor()), "instance of extended DefineMap is not a plain object");
 });
+
+if(System.env.indexOf("production") < 0) {
+	QUnit.test('Setting a value with an object type generates a warning (#148)', function() {
+		QUnit.expect(2);
+		var oldwarn = canDev.warn;
+		canDev.warn = function(mesg) {
+			QUnit.equal(mesg, "can-define: The value for options is set to an object. This will be shared by all instances of the DefineMap. Use a function that returns the object instead.");
+		};
+		//should issue a warning
+		DefineMap.extend({
+			options: {
+				value: {}
+			}
+		});
+		//should issue a warning
+		DefineMap.extend({
+			options: {
+				value: []
+			}
+		});
+
+		//should not issue a warning
+		DefineMap.extend({
+			options: {
+				value: function(){}
+			}
+		});
+
+		//should not issue a warning
+		DefineMap.extend({
+			options: {
+				value: 2
+			}
+		});
+		canDev.warn = oldwarn;
+	});
+}