Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Replace arrays and HashMap with native Map (with a fallback shim implementation) #15114

Closed
wants to merge 9 commits into from
20 changes: 18 additions & 2 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
hasOwnProperty,
createMap,
stringify,
ES6Map,

NODE_TYPE_ELEMENT,
NODE_TYPE_ATTRIBUTE,
Expand Down Expand Up @@ -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 = [];
Expand All @@ -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];
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't deleting this line break something? Every assignment of lastIndex must be beside an assignment to lastKey (and the reverse)...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._lastKey has been set to key in the previous call to _idx().
We need to re-assign this._lastIndex here, because the currently set -1 won't be true any more (since we are adding the item), but this._lastKey stays the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like you're right. That isn't really obvious just looking at this though. Worth putting it back? Or more likely just a comment?

Copy link
Member Author

@gkalpak gkalpak Sep 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be obvious if you are trying to figure out why it is being deleted, but if you look at the code (without knowing it was ever there), I think it is clearer without the comment.
I am fine adding if you still think it helps, though.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only have to reset the last* if idx <= lastIdx, I think? Not sure if it's worth any extra logic here though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true. Actually true, but this._lastIndex will be set to idx inside the _idx() call, so idx === this._lastIdx will always be true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right...

this._lastIndex = -1;
return true;
}
};

Expand Down
19 changes: 8 additions & 11 deletions src/apis.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

/* global
ES6Map */

/**
* Computes a hash of an 'obj'.
Expand Down Expand Up @@ -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 = {
Expand All @@ -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;
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/auto/injector.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
6 changes: 0 additions & 6 deletions src/ngMock/angular-mocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
59 changes: 21 additions & 38 deletions test/ApiSpecs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});
Expand Down
19 changes: 8 additions & 11 deletions test/helpers/testabilityPatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
20 changes: 10 additions & 10 deletions test/ng/directive/selectSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1515,8 +1515,8 @@ describe('select', function() {
'number:1',
'boolean:true',
'object:null',
'object:3',
'object:4',
'object:5',
'number:NaN'
);

Expand All @@ -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
]);
Expand Down Expand Up @@ -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');
});


Expand Down Expand Up @@ -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']
);
Expand Down Expand Up @@ -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']);
});

Expand Down
2 changes: 1 addition & 1 deletion test/ngAnimate/animateSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
17 changes: 0 additions & 17 deletions test/ngMock/angular-mocksSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {

Expand Down