refactor(HashMap): use ES6Map under the hood

angular · gkalpak · Oct 30, 2015 · Dec 11, 2015 · Aug 27, 2016 · Aug 28, 2016
commit fdba29734c8a1f99717378a599b777d8ca734697
diff --git a/src/Angular.js b/src/Angular.js
@@ -89,6 +89,7 @@
   hasOwnProperty,
   createMap,
   stringify,
+  ES6Map,
 
   NODE_TYPE_ELEMENT,
   NODE_TYPE_ATTRIBUTE,
@@ -752,6 +753,7 @@ function arrayRemove(array, value) {
 // A minimal ES6 Map implementation.
 // Should be bug/feature equivelent to the native implementations of supported browsers.
 // See https://kangax.github.io/compat-table/es6/#test-Map
+var nanPlaceholder = Object.create(null);
 function ES6MapShim() {
   this._keys = [];
   this._values = [];
@@ -765,23 +767,37 @@ ES6MapShim.prototype = {
     }
     return (this._lastIndex = (this._keys.indexOf(this._lastKey = key)));
   },
+  _transformKey: function(key) {
+    return isNumberNaN(key) ? nanPlaceholder : key;
+  },
   get: function(key) {
-    var idx = this._idx(key);
+    var idx = this._idx(this._transformKey(key));
     if (idx !== -1) {
       return this._values[idx];
     }
   },
   set: function(key, value) {
+    key = this._transformKey(key);
     var idx = this._idx(key);
     if (idx === -1) {
       idx = this._lastIndex = this._keys.length;
-      this._lastKey = key;
     }
     this._keys[idx] = key;
     this._values[idx] = value;
 
     // Support: IE11
     // Do not `return this` to simulate the partial IE11 implementation
+  },
+  delete: function(key) {
+    var idx = this._idx(this._transformKey(key));
+    if (idx === -1) {
+      return false;
+    }
+    this._keys.splice(idx, 1);
+    this._values.splice(idx, 1);
+    this._lastKey = NaN;
+    this._lastIndex = -1;
+    return true;
   }
 };
 

diff --git a/src/apis.js b/src/apis.js
@@ -1,5 +1,7 @@
 'use strict';
 
+/* global
+  ES6Map */
 
 /**
  * Computes a hash of an 'obj'.
@@ -36,13 +38,8 @@ function hashKey(obj, nextUidFn) {
 /**
  * HashMap which can use objects as keys
  */
-function HashMap(array, isolatedUid) {
-  if (isolatedUid) {
-    var uid = 0;
-    this.nextUid = function() {
-      return ++uid;
-    };
-  }
+function HashMap(array) {
+  this._map = new ES6Map();
   forEach(array, this.put, this);
 }
 HashMap.prototype = {
@@ -52,24 +49,24 @@ HashMap.prototype = {
    * @param value value to store can be any type
    */
   put: function(key, value) {
-    this[hashKey(key, this.nextUid)] = value;
+    this._map.set(key, value);
   },
 
   /**
    * @param key
    * @returns {Object} the value for the key
    */
   get: function(key) {
-    return this[hashKey(key, this.nextUid)];
+    return this._map.get(key);
   },
 
   /**
    * Remove the key/value pair
    * @param key
    */
   remove: function(key) {
-    var value = this[key = hashKey(key, this.nextUid)];
-    delete this[key];
+    var value = this._map.get(key);
+    this._map.delete(key);
     return value;
   }
 };

diff --git a/src/auto/injector.js b/src/auto/injector.js
@@ -649,7 +649,7 @@ function createInjector(modulesToLoad, strictDi) {
   var INSTANTIATING = {},
       providerSuffix = 'Provider',
       path = [],
-      loadedModules = new HashMap([], true),
+      loadedModules = new HashMap(),
       providerCache = {
         $provide: {
             provider: supportObject(provider),

diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js
@@ -2941,12 +2941,6 @@ angular.mock.$RootScopeDecorator = ['$delegate', function($delegate) {
       delete fn.$inject;
     });
 
-    angular.forEach(currentSpec.$modules, function(module) {
-      if (module && module.$$hashKey) {
-        module.$$hashKey = undefined;
-      }
-    });
-
     currentSpec.$injector = null;
     currentSpec.$modules = null;
     currentSpec.$providerInjector = null;

diff --git a/test/ApiSpecs.js b/test/ApiSpecs.js
@@ -5,52 +5,35 @@ describe('api', function() {
   describe('HashMap', function() {
     it('should do basic crud', function() {
       var map = new HashMap();
-      var key = {};
-      var value1 = {};
-      var value2 = {};
-      map.put(key, value1);
-      map.put(key, value2);
-      expect(map.get(key)).toBe(value2);
-      expect(map.get({})).toBeUndefined();
-      expect(map.remove(key)).toBe(value2);
-      expect(map.get(key)).toBeUndefined();
+      var keys = [{}, {}, {}];
+      var values = [{}, {}, {}];
+
+      map.put(keys[0], values[1]);
+      map.put(keys[0], values[0]);
+      expect(map.get(keys[0])).toBe(values[0]);
+      expect(map.get(keys[1])).toBeUndefined();
+
+      map.put(keys[1], values[1]);
+      map.put(keys[2], values[2]);
+      expect(map.remove(keys[0])).toBe(values[0]);
+      expect(map.remove(keys[0])).toBeUndefined();
+
+      expect(map.get(keys[1])).toBe(values[1]);
+      expect(map.get(keys[2])).toBe(values[2]);
     });
 
     it('should init from an array', function() {
-      var map = new HashMap(['a','b']);
+      var map = new HashMap(['a', 'b']);
       expect(map.get('a')).toBe(0);
       expect(map.get('b')).toBe(1);
       expect(map.get('c')).toBeUndefined();
     });
 
-    it('should maintain hashKey for object keys', function() {
-      var map = new HashMap();
-      var key = {};
-      map.get(key);
-      expect(key.$$hashKey).toBeDefined();
-    });
-
-    it('should maintain hashKey for function keys', function() {
-      var map = new HashMap();
-      var key = function() {};
-      map.get(key);
-      expect(key.$$hashKey).toBeDefined();
-    });
-
-    it('should share hashKey between HashMap by default', function() {
-      var map1 = new HashMap(), map2 = new HashMap();
-      var key1 = {}, key2 = {};
-      map1.get(key1);
-      map2.get(key2);
-      expect(key1.$$hashKey).not.toEqual(key2.$$hashKey);
-    });
-
-    it('should maintain hashKey per HashMap if flag is passed', function() {
-      var map1 = new HashMap([], true), map2 = new HashMap([], true);
-      var key1 = {}, key2 = {};
-      map1.get(key1);
-      map2.get(key2);
-      expect(key1.$$hashKey).toEqual(key2.$$hashKey);
+    it('should init from an object', function() {
+      var map = new HashMap({one: 'a', two: 'b'});
+      expect(map.get('a')).toBe('one');
+      expect(map.get('b')).toBe('two');
+      expect(map.get('c')).toBeUndefined();
     });
   });
 });

diff --git a/test/helpers/testabilityPatch.js b/test/helpers/testabilityPatch.js
@@ -54,17 +54,14 @@ beforeEach(function() {
 afterEach(function() {
   var count, cache;
 
-  // both of these nodes are persisted across tests
-  // and therefore the hashCode may be cached
-  var node = window.document.querySelector('html');
-  if (node) {
-    node.$$hashKey = null;
-  }
-  var bod = window.document.body;
-  if (bod) {
-    bod.$$hashKey = null;
-  }
-  window.document.$$hashKey = null;
+  // These Nodes are persisted across tests.
+  // They used to be assigned a `$$hashKey` when animated, but not any more.
+  var doc = window.document;
+  var html = doc.querySelector('html');
+  var body = doc.body;
+  expect(doc.$$hashKey).toBeFalsy();
+  expect(html && html.$$hashKey).toBeFalsy();
+  expect(body && body.$$hashKey).toBeFalsy();
 
   if (this.$injector) {
     var $rootScope = this.$injector.get('$rootScope');

diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js
@@ -1515,8 +1515,8 @@ describe('select', function() {
               'number:1',
               'boolean:true',
               'object:null',
+              'object:3',
               'object:4',
-              'object:5',
               'number:NaN'
             );
 
@@ -1541,15 +1541,15 @@ describe('select', function() {
             browserTrigger(element, 'change');
 
             var arrayVal = ['a'];
-            arrayVal.$$hashKey = 'object:5';
+            arrayVal.$$hashKey = 'object:4';
 
             expect(scope.selected).toEqual([
               'string',
               undefined,
               1,
               true,
               null,
-              {prop: 'value', $$hashKey: 'object:4'},
+              {prop: 'value', $$hashKey: 'object:3'},
               arrayVal,
               NaN
             ]);
@@ -1862,10 +1862,10 @@ describe('select', function() {
           scope.$digest();
 
           optionElements = element.find('option');
-          expect(element.val()).toBe(prop === 'ngValue' ? 'object:4' : 'C');
+          expect(element.val()).toBe(prop === 'ngValue' ? 'object:3' : 'C');
           expect(optionElements.length).toEqual(3);
           expect(optionElements[2].selected).toBe(true);
-          expect(scope.obj.value).toEqual(prop === 'ngValue' ? {name: 'C', $$hashKey: 'object:4'} : 'C');
+          expect(scope.obj.value).toEqual(prop === 'ngValue' ? {name: 'C', $$hashKey: 'object:3'} : 'C');
       });
 
 
@@ -2174,9 +2174,9 @@ describe('select', function() {
             expect(optionElements.length).toEqual(4);
             expect(scope.obj.value).toEqual(prop === 'ngValue' ?
               [
-                {name: 'A', $$hashKey: 'object:4', disabled: true},
-                {name: 'C', $$hashKey: 'object:6'},
-                {name: 'D', $$hashKey: 'object:7', disabled: true}
+                {name: 'A', $$hashKey: 'object:3', disabled: true},
+                {name: 'C', $$hashKey: 'object:5'},
+                {name: 'D', $$hashKey: 'object:6', disabled: true}
               ] :
               ['A', 'C', 'D']
             );
@@ -2228,13 +2228,13 @@ describe('select', function() {
             scope.$digest();
 
             optionElements = element.find('option');
-            expect(element.val()).toEqual(prop === 'ngValue' ? ['object:4', 'object:5'] : ['B', 'C']);
+            expect(element.val()).toEqual(prop === 'ngValue' ? ['object:4', 'object:7'] : ['B', 'C']);
             expect(optionElements.length).toEqual(3);
             expect(optionElements[1].selected).toBe(true);
             expect(optionElements[2].selected).toBe(true);
             expect(scope.obj.value).toEqual(prop === 'ngValue' ?
               [{ name: 'B', $$hashKey: 'object:4'},
-                {name: 'C', $$hashKey: 'object:5'}] :
+                {name: 'C', $$hashKey: 'object:7'}] :
               ['B', 'C']);
         });
 

diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js
@@ -1385,7 +1385,7 @@ describe('animations', function() {
         expect(capturedAnimation[1]).toBe('leave');
 
         // $$hashKey causes comparison issues
-        expect(element.parent()[0]).toEqual(parent[0]);
+        expect(element.parent()[0]).toBe(parent[0]);
 
         options = capturedAnimation[2];
         expect(options.addClass).toEqual('pink');

diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js
@@ -795,23 +795,6 @@ describe('ngMock', function() {
           });
         });
 
-        describe('module cleanup', function() {
-          function testFn() {
-
-          }
-
-          it('should add hashKey to module function', function() {
-            module(testFn);
-            inject(function() {
-              expect(testFn.$$hashKey).toBeDefined();
-            });
-          });
-
-          it('should cleanup hashKey after previous test', function() {
-            expect(testFn.$$hashKey).toBeUndefined();
-          });
-        });
-
         describe('$inject cleanup', function() {
           function testFn() {