Skip to content

Commit

Permalink
468 remove false warning if only default type (#469)
Browse files Browse the repository at this point in the history 
* remove false warning if only  default type

* update define/type warning tests to testCase list

* get warning test combos to pass

* add Default - zeroArgGet - set test

* breakout if test conditions into variables

* remove es6 get definitions

* fix if condition variable assignments

* fix line break jshint error
  • Loading branch information
Mike 'mitch' Mitchel authored Jul 17, 2019
1 parent f9e3d05 commit 9a35ef9
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 163 deletions.
17 changes: 12 additions & 5 deletions can-define.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,22 @@ define.property = function(typePrototype, prop, definition, dataInitializers, co

//!steal-remove-start
if(process.env.NODE_ENV !== 'production') {
if(definition.get && definition.get.length === 0 && ( "default" in definition || "Default" in definition ) ) {
canLogDev.warn("can-define: Default value for property " +
var hasZeroArgGetter = definition.get && definition.get.length === 0;
var noSetter = !definition.set;
var defaultInDefinition = ( "default" in definition || "Default" in definition );
var typeInDefinition = (definition.type && defaultDefinition && definition.type !== defaultDefinition.type) ||
(definition.Type && defaultDefinition && definition.Type !== defaultDefinition.Type);

if(hasZeroArgGetter && noSetter && defaultInDefinition) {
var defaultOrDefault = "default" in definition ? "default" : "Default";
canLogDev.warn("can-define: " + defaultOrDefault + " value for property " +
canReflect.getName(typePrototype)+"."+ prop +
" ignored, as its definition has a zero-argument getter");
}

if(definition.get && definition.get.length === 0 && ( definition.type || definition.Type ) ) {
var warning = definition.type ? 'type' : 'Type';
canLogDev.warn("can-define: " + warning + " value for property " +
if(hasZeroArgGetter && noSetter && typeInDefinition) {
var typeOrType = definition.type ? "type" : "Type";
canLogDev.warn("can-define: " + typeOrType + " value for property " +
canReflect.getName(typePrototype)+"."+ prop +
" ignored, as its definition has a zero-argument getter");
}
Expand Down
289 changes: 131 additions & 158 deletions test/test-define-only.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1182,164 +1182,6 @@ QUnit.test('defined properties are configurable', function(assert) {
assert.equal(a.val, "bar", "It was redefined");
});

testHelpers.dev.devOnlyTest("Setting a value with only a get() generates a warning (#202)", function (assert) {
assert.expect(7);

var VM = function() {};

var message = "can-define: Set value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter and no setter";
var finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {

assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});


define(VM.prototype, {
derivedProp: {
get: function() {
return "Hello World";
}
}
});

var vm = new VM();
vm.on("derivedProp", function() {});

vm.derivedProp = 'prop is set';
assert.equal(vm.derivedProp, "Hello World", "Getter value is preserved");

VM.shortName = "VM";

assert.equal(finishErrorCheck(), 1);

message = "can-define: Set value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter and no setter";
finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {
assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});

vm.derivedProp = 'prop is set';
assert.equal(finishErrorCheck(), 1);
});

testHelpers.dev.devOnlyTest("Setter with getter that doesn't take `lastSet` warns (#367)", function (assert) {
assert.expect(3);

var VM = function() {};

var message = "can-define: Set value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter";
var finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {

assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});


define(VM.prototype, {
derivedProp: {
set: function(testVar) {
return testVar;
},
get: function() {
return "Bitovi Is Awesome";
}
}
});

assert.equal(finishErrorCheck(), 1);

});

testHelpers.dev.devOnlyTest("Getter with Type or type that doesn't take `lastSet` should warn (#465)", function (assert) {
assert.expect(6);

var VM = function() {};

var message = "can-define: Type value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter";
var finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {

assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});

define(VM.prototype, {
derivedProp: {
get: function() {
return 'someString';
},
Type: { foo: 'string' }
}
});

assert.equal(finishErrorCheck(), 1);

message = "can-define: type value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter";
finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {

assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});

define(VM.prototype, {
derivedProp: {
get: function() {
return 'someString';
},
type: 'string'
}
});

assert.equal(finishErrorCheck(), 1);

});

testHelpers.dev.devOnlyTest("Default with getter that doesn't take `lastSet` warns (#367)", function (assert) {
assert.expect(6);

var VM = function() {};

var message = "can-define: Default value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter";

var finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {

assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});


define(VM.prototype, {
derivedProp: {
default: 'Amechi Egbe',
get: function() {
return "I Love Software";
}
}
});

assert.equal(finishErrorCheck(), 1, 'There was 1 warning');

message = "can-define: Default value for property "+canReflect.getName(VM.prototype)+".derivedProp ignored, as its definition has a zero-argument getter";
finishErrorCheck = testHelpers.dev.willWarn(message, function(actualMessage, success) {

assert.equal(actualMessage, message, "Warning is expected message");
assert.ok(success);
});


define(VM.prototype, {
derivedProp: {
default: '',
get: function() {
return "I Love Software";
}
}
});

assert.equal(finishErrorCheck(), 1, 'There was 1 warning');

});

testHelpers.dev.devOnlyTest("warn on using a Constructor for small-t type definitions", function (assert) {
assert.expect(1);

Expand Down Expand Up @@ -1558,3 +1400,134 @@ testHelpers.dev.devOnlyTest("warning when setting during a get (setter)", functi
inst.prop2 = "quux";
teardownWarn();
});

testHelpers.dev.devOnlyTest("warnings are given when type or default is ignored", function(assert) {
var testCases = [
{
name: "zero-arg getter, no setter when property is set",
definition: {
get: function() { return "whatever"; }
},
warning: /Set value for property .* ignored/,
setProp: true,
expectedWarnings: 1
},
{
name: "type with zero-arg getter, no setter",
definition: {
type: String,
get: function() { return "whatever"; }
},
warning: /type value for property .* ignored/,
setProp: false,
expectedWarnings: 1
},
{
name: "Type with zero-arg getter, no setter",
definition: {
Type: {},
get: function() { return "whatever"; }
},
warning: /Type value for property .* ignored/,
setProp: false,
expectedWarnings: 1
},
{
name: "only default type with zero-arg getter, no setter - should not warn",
definition: {
get: function() { return "whatever"; }
},
warning: /type value for property .* ignored/,
setProp: false,
expectedWarnings: 0
},
{
name: "type with zero-arg getter, with setter - should not warn",
definition: {
type: String,
get: function() { return "whatever"; },
set: function (val) { return val; }
},
warning: /type value for property .* ignored/,
setProp: false,
expectedWarnings: 0
},
{
name: "Type with zero-arg getter, with setter - should not warn",
definition: {
Type: {},
get: function() { return "whatever"; },
set: function (val) { return val; }
},
warning: /Type value for property .* ignored/,
setProp: false,
expectedWarnings: 0
},
{
name: "default with zero-arg getter, no setter",
definition: {
default: "some thing",
get: function() { return "whatever"; }
},
warning: /default value for property .* ignored/,
setProp: false,
expectedWarnings: 1
},
{
name: "Default with zero-arg getter, no setter",
definition: {
Default: function () {},
get: function() { return "whatever"; }
},
warning: /Default value for property .* ignored/,
setProp: false,
expectedWarnings: 1
},
{
name: "default with zero-arg getter, with setter - should not warn",
definition: {
default: "some thing",
get: function() { return "whatever"; },
set: function (val) { return val; }
},
warning: /default value for property .* ignored/,
setProp: false,
expectedWarnings: 0
},
{
name: "Default with zero-arg getter, with setter - should not warn",
definition: {
Default: function () {},
get: function() { return "whatever"; },
set: function (val) { return val; }
},
warning: /Default value for property .* ignored/,
setProp: false,
expectedWarnings: 0
}
];

testCases.forEach(function(testCase) {
var VM = function() {};
var warnCount = testHelpers.dev.willWarn(testCase.warning);

define(VM.prototype, {
derivedProp: testCase.definition,
"*": { // emulates can-define/map/map setting default type
type: define.types.observable
}
});

var vm = new VM();

// read prop for 'lazy' setup
canReflect.onKeyValue(vm, 'derivedProp', function() {});

if (testCase.setProp) {
vm.derivedProp = "smashed it!";
}


assert.equal(warnCount(), testCase.expectedWarnings, "got correct number of warnings for " + testCase.name);
});
});

0 comments on commit 9a35ef9

Please sign in to comment.