From 5a04e5eb9f83519db8549ddd0c6069bc952f5e74 Mon Sep 17 00:00:00 2001 From: Bradley Momberger Date: Tue, 29 Aug 2017 16:16:57 -0400 Subject: [PATCH 1/3] Warn when setting a property that has only a zero-arg getter. (#202) --- can-define.js | 13 ++++++++++++- define-test.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/can-define.js b/can-define.js index 1082858..4c9e849 100644 --- a/can-define.js +++ b/can-define.js @@ -253,11 +253,21 @@ 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("Set value for property " + + prop + + " ignored, as its definition has a zero-argument getter and no setter"); + }; + } + //!steal-remove-end // Add type behavior to the setter. if (type) { @@ -326,6 +336,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 index 2d94fc4..f6c76e2 100644 --- a/define-test.js +++ b/define-test.js @@ -5,6 +5,7 @@ var CanList = require("can-define/list/list"); var canBatch = require("can-event/batch/batch"); var each = require("can-util/js/each/each"); var canSymbol = require("can-symbol"); +var canDev = require("can-util/js/dev/dev"); QUnit.module("can-define"); @@ -1368,3 +1369,32 @@ QUnit.test('define() should add a CID (#246)', function() { var g = new Greeting(); QUnit.ok(g._cid, "should have a CID property"); }); + +if(System.env.indexOf("production") < 0) { + QUnit.test('Setting a value with only a get() generates a warning (#202)', function() { + QUnit.expect(2); + 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, + "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"); + canDev.warn = oldwarn; + }); +} \ No newline at end of file From 97f89bd61cd1bbfc3d3f8ad8cf62e7523ee08b03 Mon Sep 17 00:00:00 2001 From: Bradley Momberger Date: Wed, 30 Aug 2017 20:03:42 -0400 Subject: [PATCH 2/3] Improve warning message for get-with-no-set condition --- can-define.js | 3 ++- define-test.js | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/can-define.js b/can-define.js index 4c9e849..6d683e0 100644 --- a/can-define.js +++ b/can-define.js @@ -262,8 +262,9 @@ define.property = function(objPrototype, prop, definition, dataInitializers, com // 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("Set value for property " + + 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"); }; } diff --git a/define-test.js b/define-test.js index f6c76e2..0188590 100644 --- a/define-test.js +++ b/define-test.js @@ -1372,7 +1372,7 @@ QUnit.test('define() should add a CID (#246)', function() { if(System.env.indexOf("production") < 0) { QUnit.test('Setting a value with only a get() generates a warning (#202)', function() { - QUnit.expect(2); + QUnit.expect(3); var VM = function() {}; define(VM.prototype, { derivedProp: { @@ -1389,12 +1389,22 @@ if(System.env.indexOf("production") < 0) { canDev.warn = function(mesg) { QUnit.equal( mesg, - "Set value for property derivedProp ignored, as its definition has a zero-argument getter and no setter", + "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; }); } \ No newline at end of file From c3df182267951e3d12d1c5d5e739c6218583aa5c Mon Sep 17 00:00:00 2001 From: Bradley Momberger Date: Thu, 31 Aug 2017 11:34:58 -0400 Subject: [PATCH 3/3] Fix warning tests based on other merged PRs --- define-test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/define-test.js b/define-test.js index 857e31c..8e6d890 100644 --- a/define-test.js +++ b/define-test.js @@ -1426,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({}); + } } }); @@ -1439,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({}); + } } });