Show a warning for incorrect value for component setters. Fix some TODOs

CHANGELOG.md

@@ -9,6 +9,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 * Show a warning when a stage has too many layers
 * Show a warning on duplicate ids
 * Show a warning on weird class in `Node.create` parsing from JSON
+* Show a warning for incorrect value for component setters. 
 
 ### Changed
 * Fixes inconsistent `layer.setSize()` method. Now it has same arguments as any container.

src/BaseLayer.ts

@@ -59,7 +59,8 @@ export abstract class BaseLayer extends Container {
     return this.getCanvas().getContext();
   }
   /**
-   * clear scene and hit canvas contexts tied to the layer
+   * clear scene and hit canvas contexts tied to the layer.
+   * This function doesn't remove any nodes. It just clear canvas element.
    * @method
    * @name Konva.BaseLayer#clear
    * @param {Object} [bounds]

src/Factory.ts

@@ -1,5 +1,6 @@
 import { isUnminified } from './Global';
 import { Util } from './Util';
+import Konva from './index';
 
 var GET = 'get',
   SET = 'set';
@@ -62,6 +63,8 @@ export const Factory = {
       return ret;
     };
 
+    var basicValidator = Validators.getComponentValidator(components);
+
     // setter
     constructor.prototype[setter] = function(val) {
       var oldVal = this.attrs[attr],
@@ -71,6 +74,10 @@ export const Factory = {
         val = validator.call(this, val);
       }
 
+      if (basicValidator) {
+        basicValidator.call(this, val, attr);
+      }
+
       for (key in val) {
         if (!val.hasOwnProperty(key)) {
           continue;
@@ -288,5 +295,21 @@ export const Validators = {
         return val;
       };
     }
+  },
+  getComponentValidator(components) {
+    if (isUnminified) {
+      return function(val, attr) {
+        if (!Util.isObject(val)) {
+          Util.warn(
+            Validators._formatValue(val) +
+              ' is a not valid value for "' +
+              attr +
+              '" attribute. The value should be an object with properties ' +
+              components
+          );
+        }
+        return val;
+      };
+    }
   }
 };

src/Node.ts

@@ -2157,6 +2157,7 @@ export abstract class Node {
     } else {
       this.on('mousedown.konva touchstart.konva', function(evt) {
         // ignore right and middle buttons
+        // TODO: should we add a global config for buttons?
         if (evt.evt.button === 1 || evt.evt.button === 2) {
           return;
         }
@@ -2225,7 +2226,7 @@ export abstract class Node {
   enhance: GetSet<number, this>;
   filters: GetSet<Filter[], this>;
   position: GetSet<Vector2d, this>;
-  size: GetSet<Vector2d, this>;
+  size: GetSet<{ width: number; height: number }, this>;
 
   id: GetSet<string, this>;
 
@@ -2441,7 +2442,6 @@ Factory.addGetterSetter(Node, 'opacity', 1, Validators.getNumberValidator());
  * node.opacity(0.5);
  */
 
-// TODO: should default name be empty string?
 Factory.addGetterSetter(Node, 'name', '', Validators.getStringValidator());
 
 /**
@@ -2881,7 +2881,7 @@ Factory.addGetterSetter(Node, 'size');
  *   // important pos - is absolute position of the node
  *   // you should return absolute position too
  *   return {
- *     x: this.getAbsolutePosition().x,
+ *     x: this.absolutePosition().x,
  *     y: pos.y
  *   };
  * });

src/Shape.ts

@@ -179,7 +179,6 @@ export class Shape extends Node {
     return (
       this.strokeEnabled() &&
       !!(this.stroke() || this.strokeLinearGradientColorStops())
-      // TODO: do we need radial gradient
       // this.getStrokeRadialGradientColorStops()
     );
   }
@@ -216,8 +215,10 @@ export class Shape extends Node {
     delete shapes[this.colorKey];
     return this;
   }
-  // TODO: write why do we need it,
-  // try to use it without stage (use global buffer canvas)
+  // why do we need buffer canvas?
+  // it give better result when a shape has
+  // stroke with fill and with some opacity
+  // TODO: try to use it without stage (use global buffer canvas)
   _useBufferCanvas(caching) {
     return (
       (!caching || this.hasShadow()) &&
@@ -241,7 +242,7 @@ export class Shape extends Node {
    *
    */
   getSelfRect() {
-    var size = this.getSize();
+    var size = this.size();
     return {
       x: this._centroid ? Math.round(-size.width / 2) : 0,
       y: this._centroid ? Math.round(-size.height / 2) : 0,

src/Stage.ts

@@ -7,7 +7,6 @@ import { GetSet, Vector2d } from './types';
 import { Shape } from './Shape';
 import { BaseLayer } from './BaseLayer';
 
-// TODO: add a warning if stage has too many layers
 // TODO: remove "content" events from docs
 
 // CONSTANTS
@@ -110,7 +109,6 @@ export class Stage extends Container {
 
   constructor(config) {
     super(config);
-    this.nodeType = STAGE;
     this._buildDOM();
     this._bindContentEvents();
     stages.push(this);
@@ -745,7 +743,6 @@ export class Stage extends Container {
     }
   }
   // currently cache function is now working for stage, because stage has no its own canvas element
-  // TODO: may be it is better to cache all children layers?
   cache() {
     Util.warn(
       'Cache function is not allowed for stage. You may use cache only for layers, groups and shapes.'
@@ -771,6 +768,8 @@ export class Stage extends Container {
   container: GetSet<HTMLDivElement, this>;
 }
 
+Stage.prototype.nodeType = STAGE;
+
 // TODO: test for replacing container
 Factory.addGetterSetter(Stage, 'container');
 

src/shapes/Label.ts

@@ -305,8 +305,7 @@ export class Tag extends Shape {
     };
   }
 
-  // TODO: not a string, but a strict set
-  pointerDirection: GetSet<string, this>;
+  pointerDirection: GetSet<'left' | 'top' | 'right' | 'bottom', this>;
   pointerWidth: GetSet<number, this>;
   pointerHeight: GetSet<number, this>;
   cornerRadius: GetSet<number, this>;

src/shapes/Text.ts

@@ -188,7 +188,8 @@ export class Text extends Shape {
             ? totalWidth - padding * 2
             : width;
         context.lineTo(Math.round(lineWidth), Math.round(lineHeightPx / 2));
-        // TODO: I have no idea what is real ratio
+
+        // I have no idea what is real ratio
         // just /15 looks good enough
         context.lineWidth = fontSize / 15;
         context.strokeStyle = fill;
@@ -286,7 +287,7 @@ export class Text extends Shape {
     return this.textHeight;
   }
 
-  // TODO: make it public?
+  // TODO: make it public, rename to "measure text"?
   _getTextSize(text) {
     var _context = getDummyContext(),
       fontSize = this.fontSize(),

src/shapes/TextPath.ts

@@ -433,7 +433,7 @@ export class TextPath extends Shape {
     };
 
     // fake search for offset, this is very bad approach
-    // TODO: find other way to add offset from start (for align)
+    // find other way to add offset from start (for align)
     var testChar = 'C';
     var glyphWidth = that._getTextSize(testChar).width + letterSpacing;
     for (var k = 0; k < offset / glyphWidth; k++) {

src/shapes/Transformer.ts

@@ -6,7 +6,7 @@ import { Rect } from './Rect';
 import { Group } from '../Group';
 import { getAngle, getGlobalKonva } from '../Global';
 
-import { GetSet } from '../types';
+import { GetSet, IRect } from '../types';
 
 var ATTR_CHANGE_LIST = [
   'resizeEnabledChange',
@@ -107,7 +107,6 @@ function getCursor(anchorName, rad, isMirrored) {
     return 'nwse-resize';
   } else {
     // how can we can there?
-    // TODO: throw error
     Util.error('Transformer has unknown angle for cursor detection: ' + angle);
     return 'pointer';
   }
@@ -216,7 +215,8 @@ export class Transformer extends Group {
       }.bind(this)
     );
 
-    // TODO: why do we need this?
+    // we may need it if we set not in initial props
+    // so elements are not defined yet
     var elementsCreated = !!this.findOne('.top-left');
     if (elementsCreated) {
       this.update();
@@ -834,8 +834,7 @@ export class Transformer extends Group {
   keepRatio: GetSet<boolean, this>;
   centeredScaling: GetSet<boolean, this>;
   ignoreStroke: GetSet<boolean, this>;
-  // TODO: add better types
-  boundBoxFunc: GetSet<Function, this>;
+  boundBoxFunc: GetSet<(oldBox: IRect, newBox: IRect) => IRect, this>;
 }
 
 function validateAnchors(val) {

test/unit/Node-test.js

@@ -3789,4 +3789,18 @@ suite('Node', function() {
     assert.equal(layer.getChildren().length, 1);
     assert.equal(rect3.getZIndex(), 0);
   });
+
+  test('show warning when we are trying to use non-objects for component setters', function() {
+    var stage = addStage();
+
+    var callCount = 0;
+    var oldWarn = Konva.Util.warn;
+    Konva.Util.warn = function() {
+      callCount += 1;
+    };
+
+    stage.scale(0.5);
+    assert.equal(callCount, 1);
+    Konva.Util.warn = oldWarn;
+  });
 });