warning on dublicate ids

CHANGELOG.md

@@ -7,13 +7,14 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Added
 * Show a warning when a stage has too many layers.
+* Show a warning on duplicate ids
 
 ### Changed
 * Fixes inconsistent `layer.setSize()` method. Now it has same arguments as any container.
 * Full rewrite to Typescript with tons of refactoring and small optimizations. The public API should be 100% the same
 * Fixed `patternImage` and `radialGradient` for `Konva.Text`
 * `Konva.Util._isObject` is renamed to `Konva.Util._isPlainObject`.
-* TODO: changed behavior of `removeId`.
+* changed behavior of `removeId`.
 
 ### Removed
 * `Konva.Util.addMethods`

konva.js

@@ -20,8 +20,6 @@
    * @namespace Konva
    */
   var version = '@@version';
-  // private
-  var ids = {};
   var names = {};
   var shapes = {};
   var isBrowser = typeof window !== 'undefined' &&
@@ -62,31 +60,6 @@
       }
       return false;
   };
-  var _addId = function (node, id) {
-      if (!id) {
-          return;
-      }
-      // do we need this warning?
-      // if (ids[id]) {
-      //   console.warn(
-      //     'Duplicate id "' +
-      //       id +
-      //       '". Please don not use same id several times. It may break find() method look up.'
-      //   );
-      // }
-      ids[id] = node;
-  };
-  var _removeId = function (id, node) {
-      // node has no id
-      if (!id) {
-          return;
-      }
-      // another node is registered (possible for duplicate ids)
-      if (ids[id] !== node) {
-          return;
-      }
-      delete ids[id];
-  };
   var _addName = function (node, name) {
       if (name) {
           if (!names[name]) {
@@ -2086,6 +2059,28 @@
       return HitCanvas;
   }(Canvas));
 
+  var ids = {};
+  var ID_WARNING = "Duplicate id \"{id}\".\nPlease do not use same id several times, it will break find() method look up.\nIf you have duplicates it is better to use \"name\" property instead.\n";
+  var _addId = function (node, id) {
+      if (!id) {
+          return;
+      }
+      if (ids[id]) {
+          Util.warn(ID_WARNING.replace('{id}', id));
+      }
+      ids[id] = node;
+  };
+  var _removeId = function (id, node) {
+      // node has no id
+      if (!id) {
+          return;
+      }
+      // another node is registered (possible for duplicate ids)
+      if (ids[id] !== node) {
+          return;
+      }
+      delete ids[id];
+  };
   // CONSTANTS
   var ABSOLUTE_OPACITY = 'absoluteOpacity', ABSOLUTE_TRANSFORM = 'absoluteTransform', ABSOLUTE_SCALE = 'absoluteScale', CHANGE = 'Change', CHILDREN = 'children', KONVA = 'konva', LISTENING = 'listening', MOUSEENTER = 'mouseenter', MOUSELEAVE = 'mouseleave', NAME = 'name', SET$1 = 'set', SHAPE = 'Shape', SPACE = ' ', STAGE = 'stage', TRANSFORM = 'transform', UPPER_STAGE = 'Stage', VISIBLE = 'visible', CLONE_BLACK_LIST = ['id'], TRANSFORM_CHANGE_STR = [
       'xChange.konva',
@@ -15650,6 +15645,7 @@
     Collection: Collection,
     Util: Util,
     Node: Node,
+    ids: ids,
     Container: Container,
     Stage: Stage,
     stages: stages,
@@ -15680,7 +15676,6 @@
     Transformer: Transformer,
     Wedge: Wedge,
     version: version,
-    ids: ids,
     names: names,
     shapes: shapes,
     isBrowser: isBrowser,
@@ -15688,8 +15683,6 @@
     dblClickWindow: dblClickWindow,
     isDragging: isDragging,
     isDragReady: isDragReady,
-    _addId: _addId,
-    _removeId: _removeId,
     _addName: _addName,
     _removeName: _removeName,
     getAngle: getAngle,

src/Container.ts

@@ -1,9 +1,9 @@
 import { Util, Collection } from './Util';
 import { Factory, Validators } from './Factory';
-import { Node } from './Node';
+import { Node, ids } from './Node';
-import { getGlobalKonva, ids, names } from './Global';
+import { getGlobalKonva, names } from './Global';
 
-import { GetSet, Vector2d, IRect } from './types';
+import { GetSet, IRect } from './types';
 
 /**
  * Container constructor.  Containers are used to contain nodes or other containers

src/Global.ts

@@ -15,8 +15,6 @@ var PI_OVER_180 = Math.PI / 180;
  */
 export const version = '@@version';
 
-// private
-export const ids = {};
 export const names = {};
 export const shapes = {};
 
@@ -66,32 +64,6 @@ export const isDragReady = function() {
   }
   return false;
 };
-export const _addId = function(node: any, id) {
-  if (!id) {
-    return;
-  }
-  // do we need this warning?
-  // if (ids[id]) {
-  //   console.warn(
-  //     'Duplicate id "' +
-  //       id +
-  //       '". Please don not use same id several times. It may break find() method look up.'
-  //   );
-  // }
-  ids[id] = node;
-};
-
-export const _removeId = function(id: string, node: any) {
-  // node has no id
-  if (!id) {
-    return;
-  }
-  // another node is registered (possible for duplicate ids)
-  if (ids[id] !== node) {
-    return;
-  }
-  delete ids[id];
-};
 
 export const _addName = function(node: any, name) {
   if (name) {

src/Node.ts

@@ -1,16 +1,39 @@
 import { Util, Collection, Transform, RectConf, Point } from './Util';
 import { Factory, Validators } from './Factory';
 import { SceneCanvas, HitCanvas } from './Canvas';
-import {
+import { _removeName, _addName, getGlobalKonva } from './Global';
-  _removeId,
-  _removeName,
-  _addId,
-  _addName,
-  getGlobalKonva
-} from './Global';
 import { Container } from './Container';
 import { GetSet, Vector2d } from './types';
 
+export const ids = {};
+
+const ID_WARNING = `Duplicate id "{id}".
+Please do not use same id several times, it will break find() method look up.
+If you have duplicates it is better to use "name" property instead.
+`;
+
+const _addId = function(node: Node, id) {
+  if (!id) {
+    return;
+  }
+  if (ids[id]) {
+    Util.warn(ID_WARNING.replace('{id}', id));
+  }
+  ids[id] = node;
+};
+
+export const _removeId = function(id: string, node: any) {
+  // node has no id
+  if (!id) {
+    return;
+  }
+  // another node is registered (possible for duplicate ids)
+  if (ids[id] !== node) {
+    return;
+  }
+  delete ids[id];
+};
+
 export type Filter = (this: Node, imageData: ImageData) => void;
 
 type globalCompositeOperationType =

src/internals.ts

@@ -1,7 +1,7 @@
 export * from './Global';
 
 export { Collection, Util } from './Util';
-export { Node } from './Node';
+export { Node, ids } from './Node';
 export { Container } from './Container';
 
 export { Stage, stages } from './Stage';

test/unit/Node-test.js

@@ -118,20 +118,23 @@ suite('Node', function() {
     assert.equal(layer.getAbsoluteOpacity(), 0.5);
   });
 
-  test.skip('warn on duplicate id', function() {
+  test.only('warn on duplicate id', function() {
     var oldWarn = Konva.Util.warn;
     var called = false;
     Konva.Util.warn = function() {
       called = true;
     };
-    var circle = new Konva.Circle({
+    var circle1 = new Konva.Circle({
       id: 'circle'
     });
-    var circle = new Konva.Circle({
+    var circle2 = new Konva.Circle({
       id: 'circle'
     });
+
     assert.equal(called, true);
     Konva.Util.warn = oldWarn;
+    circle1.destroy();
+    circle2.destroy();
   });
 
   // ======================================================
@@ -1153,8 +1156,6 @@ suite('Node', function() {
     layer.add(circle);
     stage.add(layer);
 
-    
-
     /*
      * add custom attr that points to self.  The setAttrs method should
      * not inifinitely recurse causing a stack overflow
@@ -3085,7 +3086,7 @@ suite('Node', function() {
 
     // last shape is registered
     assert.equal(Konva.ids.shape, rect);
-    
+
     // destroying circle should not remove rect from regiter
     circle.destroy();
 

test/unit/Stage-test.js

@@ -1231,12 +1231,11 @@ suite('Stage', function() {
     image.src = url;
   });
 
-  test.only('show a warning if the stage has too many layers', function() {
+  test('show a warning if the stage has too many layers', function() {
     var stage = addStage();
     var oldWarn = Konva.Util.warn;
     var called = false;
     Konva.Util.warn = function() {
-      oldWarn.apply(null, arguments);
       called = true;
     };