Skip to content

Commit

Permalink
Warn when setting a property that has only a zero-arg getter. (#202)
Browse files Browse the repository at this point in the history
  • Loading branch information
bmomberger-bitovi committed Aug 29, 2017
1 parent cae36ab commit 5a04e5e
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
13 changes: 12 additions & 1 deletion can-define.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

This comment has been minimized.

Copy link
@justinbmeyer

justinbmeyer Oct 27, 2017

Contributor

this causes an error in the production tests. I don't think we should be warning. We should probably be throwing no matter what.

This comment has been minimized.

Copy link
@justinbmeyer

justinbmeyer Oct 27, 2017

Contributor

I moved the //!steal-removes to wrap the dev.warn call

This comment has been minimized.

Copy link
@phillipskevin

phillipskevin Oct 27, 2017

Contributor

why does this cause an error in the production tests?

This comment has been minimized.

Copy link
@phillipskevin

phillipskevin Oct 27, 2017

Contributor
// 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) {
Expand Down Expand Up @@ -326,6 +336,7 @@ make = {
handler: function(newVal) {
var oldValue = computeObj.oldValue;
computeObj.oldValue = newVal;

canEvent.dispatch.call(map, {
type: prop,
target: map,
Expand Down
30 changes: 30 additions & 0 deletions define-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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;
});
}

0 comments on commit 5a04e5e

Please sign in to comment.