Merge pull request #256 from canjs/202-warn-on-ignored-set

Warn when setting a property that has only a zero-arg getter.
canjs · Aug 31, 2017 · 21787bb · 21787bb
2 parents f51e93a + c3df182
commit 21787bb
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 4 deletions.
diff --git a/can-define.js b/can-define.js
@@ -271,11 +271,22 @@ define.property = function(objPrototype, prop, definition, dataInitializers, com
 		// Add `set` functionality to the eventSetter.
 		setter = make.set.setter(prop, definition.set, reader, eventsSetter, false);
 	}
-	// If there's niether `set` or `get`,
+	// If there's neither `set` or `get`,
 	else if (!definition.get) {
 		// make a set that produces events.
 		setter = eventsSetter;
 	}
+	//!steal-remove-start
+	// If there's zero-arg `get` but not `set`, warn on all sets in dev mode
+	else if (definition.get.length < 1) {
+		setter = function() {
+			dev.warn("can-define: Set value for property " +
+				prop +
+				(objPrototype.constructor.shortName ? " on " + objPrototype.constructor.shortName : "") +
+				" ignored, as its definition has a zero-argument getter and no setter");
+		};
+	}
+	//!steal-remove-end
 
 	// Add type behavior to the setter.
 	if (type) {
@@ -344,6 +355,7 @@ make = {
 				handler: function(newVal) {
 					var oldValue = computeObj.oldValue;
 					computeObj.oldValue = newVal;
+
 					canEvent.dispatch.call(map, {
 						type: prop,
 						target: map,

diff --git a/define-test.js b/define-test.js
@@ -1370,7 +1370,43 @@ QUnit.test('define() should add a CID (#246)', function() {
 	QUnit.ok(g._cid, "should have a CID property");
 });
 
-if (System.env.indexOf("production") < 0) {
+if(System.env.indexOf("production") < 0) {
+	QUnit.test('Setting a value with only a get() generates a warning (#202)', function() {
+		QUnit.expect(3);
+		var VM = function() {};
+		define(VM.prototype, {
+			derivedProp: {
+				get: function() {
+					return "Hello World";
+				}
+			}
+		});
+
+		var vm = new VM();
+		vm.on("derivedProp", function() {});
+
+		var oldwarn = canDev.warn;
+		canDev.warn = function(mesg) {
+			QUnit.equal(
+				mesg,
+				"can-define: Set value for property derivedProp ignored, as its definition has a zero-argument getter and no setter",
+				"Warning is expected message");
+		};
+
+		vm.derivedProp = 'prop is set';
+		QUnit.equal(vm.derivedProp, "Hello World", "Getter value is preserved");
+
+		VM.shortName = "VM";
+		canDev.warn = function(mesg) {
+			QUnit.equal(
+				mesg,
+				"can-define: Set value for property derivedProp on VM ignored, as its definition has a zero-argument getter and no setter",
+				"Warning is expected message");
+		};
+
+		vm.derivedProp = 'prop is set';
+		canDev.warn = oldwarn;
+	});
 
 	QUnit.test("warn on using a Constructor for small-t type definintions", function() {
 		expect(2);
@@ -1390,7 +1426,9 @@ if (System.env.indexOf("production") < 0) {
 		define(VM.prototype, {
 		    currency: {
 		        type: Currency, // should be `Type: Currency`
-		        value: new Currency({})
+		        value: function() {
+		        	return new Currency({});
+		        }
 		    }
 		});
 
@@ -1403,7 +1441,9 @@ if (System.env.indexOf("production") < 0) {
 		define(VM2.prototype, {
 		    currency: {
 		        type: Currency, // should be `Type: Currency`
-		        value: new Currency({})
+		        value: function() {
+		        	return new Currency({});
+		        }
 		    }
 		});