diff --git a/can-define.js b/can-define.js index 6d683e0..9cd0cc5 100644 --- a/can-define.js +++ b/can-define.js @@ -182,6 +182,17 @@ define.property = function(objPrototype, prop, definition, dataInitializers, com var type = definition.type; + //!steal-remove-start + if (type && canReflect.isConstructorLike(type)) { + dev.warn( + "can-define: the definition for " + + prop + + (objPrototype.constructor.shortName ? " on " + objPrototype.constructor.shortName : "") + + " uses a constructor for \"type\". Did you mean \"Type\"?" + ); + } + //!steal-remove-end + // Special case definitions that have only `type: "*"`. if (type && onlyType(definition) && type === define.types["*"]) { Object.defineProperty(objPrototype, prop, { @@ -226,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)); } @@ -856,40 +874,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/define-test.js b/define-test.js index 0188590..857e31c 100644 --- a/define-test.js +++ b/define-test.js @@ -1407,4 +1407,43 @@ if(System.env.indexOf("production") < 0) { vm.derivedProp = 'prop is set'; canDev.warn = oldwarn; }); + + QUnit.test("warn on using a Constructor for small-t type definintions", function() { + expect(2); + var oldWarn = canDev.warn; + canDev.warn = function(mesg) { + QUnit.equal(mesg, "can-define: the definition for currency uses a constructor for \"type\". Did you mean \"Type\"?"); + }; + + function Currency() { + return this; + } + Currency.prototype = { + symbol: "USD" + }; + + function VM() {} + define(VM.prototype, { + currency: { + type: Currency, // should be `Type: Currency` + value: new Currency({}) + } + }); + + canDev.warn = function(mesg) { + QUnit.equal(mesg, "can-define: the definition for currency on VM2 uses a constructor for \"type\". Did you mean \"Type\"?"); + }; + + function VM2() {} + VM2.shortName = "VM2"; + define(VM2.prototype, { + currency: { + type: Currency, // should be `Type: Currency` + value: new Currency({}) + } + }); + + canDev.warn = oldWarn; + }); + } \ No newline at end of file diff --git a/map/map-test.js b/map/map-test.js index b106333..a01d2cf 100644 --- 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; + }); +} diff --git a/package.json b/package.json index 21cbaa3..a372916 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ }, "devDependencies": { "jshint": "^2.9.1", - "serve": "^5.1.4", + "serve": "^6.0.1", "steal": "^1.0.7", "steal-qunit": "^1.0.0", "steal-tools": "^1.4.0",