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

refactor(*): replace HashMap with NgMap #15483

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@

/* apis.js */
"hashKey": false,
"HashMap": false,
"NgMap": false,

/* urlUtils.js */
"urlResolve": false,
Expand Down
4 changes: 2 additions & 2 deletions src/AngularPublic.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@
$$ForceReflowProvider,
$InterpolateProvider,
$IntervalProvider,
$$HashMapProvider,
$HttpProvider,
$HttpParamSerializerProvider,
$HttpParamSerializerJQLikeProvider,
Expand All @@ -79,6 +78,7 @@
$jsonpCallbacksProvider,
$LocationProvider,
$LogProvider,
$$MapProvider,
$ParseProvider,
$RootScopeProvider,
$QProvider,
Expand Down Expand Up @@ -260,7 +260,7 @@ function publishExternalAPI(angular) {
$window: $WindowProvider,
$$rAF: $$RAFProvider,
$$jqLite: $$jqLiteProvider,
$$HashMap: $$HashMapProvider,
$$Map: $$MapProvider,
$$cookieReader: $$CookieReaderProvider
});
}
Expand Down
91 changes: 55 additions & 36 deletions src/apis.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';


/**
* Computes a hash of an 'obj'.
* Hash of a:
Expand Down Expand Up @@ -33,49 +32,69 @@ function hashKey(obj, nextUidFn) {
return key;
}

/**
* HashMap which can use objects as keys
*/
function HashMap(array, isolatedUid) {
if (isolatedUid) {
var uid = 0;
this.nextUid = function() {
return ++uid;
};
}
forEach(array, this.put, this);
// A minimal ES2015 Map implementation.
// Should be bug/feature equivalent to the native implementations of supported browsers
// (for the features required in Angular).
// See https://kangax.github.io/compat-table/es6/#test-Map
var nanKey = Object.create(null);
function NgMapShim() {
this._keys = [];
this._values = [];
this._lastKey = NaN;
this._lastIndex = -1;
}
HashMap.prototype = {
/**
* Store key value pair
* @param key key to store can be any type
* @param value value to store can be any type
*/
put: function(key, value) {
this[hashKey(key, this.nextUid)] = value;
NgMapShim.prototype = {
_idx: function(key) {
if (key === this._lastKey) {
return this._lastIndex;
}
this._lastKey = key;
this._lastIndex = this._keys.indexOf(key);
return this._lastIndex;
},
_transformKey: function(key) {
return isNumberNaN(key) ? nanKey : key;
},

/**
* @param key
* @returns {Object} the value for the key
*/
get: function(key) {
return this[hashKey(key, this.nextUid)];
key = this._transformKey(key);
var idx = this._idx(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._keys[idx] = key;
this._values[idx] = value;

/**
* Remove the key/value pair
* @param key
*/
remove: function(key) {
var value = this[key = hashKey(key, this.nextUid)];
delete this[key];
return value;
// Support: IE11
// Do not `return this` to simulate the partial IE11 implementation
},
delete: function(key) {
key = this._transformKey(key);
var idx = this._idx(key);
if (idx === -1) {
return false;
}
this._keys.splice(idx, 1);
this._values.splice(idx, 1);
this._lastKey = NaN;
this._lastIndex = -1;
return true;
}
};

var $$HashMapProvider = [/** @this */function() {
// For now, always use `NgMapShim`, even if `window.Map` is available. Some native implementations
// are still buggy (often in subtle ways) and can cause hard-to-debug failures. When native `Map`
// implementations get more stable, we can reconsider switching to `window.Map` (when available).
var NgMap = NgMapShim;

var $$MapProvider = [/** @this */function() {
this.$get = [function() {
return HashMap;
return NgMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we feature detect native Map implementation and just use that if present? It should be significantly faster than this implementation since lookups are not O(n), and all modern browsers support it at this point. This fallback is really just for like...IE9 and IE10.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I just saw the comments above because reading fail. What subtle bugs in native Maps exist such that we shouldn't use them for perf, other than IE11 return values not conforming to the spec? Can we add references to filed issues such that when these bugs are fixed, we know that it's safe to migrate?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is some discussion in #15114. It is not as much about specific bugs as it is about our lack of confidence that there aren't any (or that our tests would have caught them).

Copy link
Member Author

Choose a reason for hiding this comment

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

(For example, there was some Safari 8 bug (which I couldn't even figure out what it was) that caused our tests to fail, even if the required features were theoretically available.)

Copy link
Contributor

Choose a reason for hiding this comment

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

With that logic, the native Map will never be used as there is no concrete way to judge whether or not they're "ready" - that's why I was suggesting we put some kind of comment in here that "when these things are fixed, re-evaluate"

Copy link
Member Author

Choose a reason for hiding this comment

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

"when these things are fixed, re-evaluate"

Sounds great. If we only knew what these things were... 😞

}];
}];
4 changes: 2 additions & 2 deletions 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 NgMap(),
providerCache = {
$provide: {
provider: supportObject(provider),
Expand Down Expand Up @@ -757,7 +757,7 @@ function createInjector(modulesToLoad, strictDi) {
var runBlocks = [], moduleFn;
forEach(modulesToLoad, function(module) {
if (loadedModules.get(module)) return;
loadedModules.put(module, true);
loadedModules.set(module, true);

function runInvokeQueue(queue) {
var i, ii;
Expand Down
6 changes: 3 additions & 3 deletions src/ng/animate.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ var $$CoreAnimateJsProvider = /** @this */ function() {
// this is prefixed with Core since it conflicts with
// the animateQueueProvider defined in ngAnimate/animateQueue.js
var $$CoreAnimateQueueProvider = /** @this */ function() {
var postDigestQueue = new HashMap();
var postDigestQueue = new NgMap();
var postDigestElements = [];

this.$get = ['$$AnimateRunner', '$rootScope',
Expand Down Expand Up @@ -139,7 +139,7 @@ var $$CoreAnimateQueueProvider = /** @this */ function() {
jqLiteRemoveClass(elm, toRemove);
}
});
postDigestQueue.remove(element);
postDigestQueue.delete(element);
}
});
postDigestElements.length = 0;
Expand All @@ -154,7 +154,7 @@ var $$CoreAnimateQueueProvider = /** @this */ function() {

if (classesAdded || classesRemoved) {

postDigestQueue.put(element, data);
postDigestQueue.set(element, data);
postDigestElements.push(element);

if (postDigestElements.length === 1) {
Expand Down
12 changes: 6 additions & 6 deletions src/ng/directive/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var SelectController =
['$element', '$scope', /** @this */ function($element, $scope) {

var self = this,
optionsMap = new HashMap();
optionsMap = new NgMap();

self.selectValueMap = {}; // Keys are the hashed values, values the original values

Expand Down Expand Up @@ -137,7 +137,7 @@ var SelectController =
self.emptyOption = element;
}
var count = optionsMap.get(value) || 0;
optionsMap.put(value, count + 1);
optionsMap.set(value, count + 1);
// Only render at the end of a digest. This improves render performance when many options
// are added during a digest and ensures all relevant options are correctly marked as selected
scheduleRender();
Expand All @@ -148,13 +148,13 @@ var SelectController =
var count = optionsMap.get(value);
if (count) {
if (count === 1) {
optionsMap.remove(value);
optionsMap.delete(value);
if (value === '') {
self.hasEmptyOption = false;
self.emptyOption = undefined;
}
} else {
optionsMap.put(value, count - 1);
optionsMap.set(value, count - 1);
}
}
};
Expand Down Expand Up @@ -606,9 +606,9 @@ var selectDirective = function() {

// Write value now needs to set the selected property of each matching option
selectCtrl.writeValue = function writeMultipleValue(value) {
var items = new HashMap(value);
forEach(element.find('option'), function(option) {
option.selected = isDefined(items.get(option.value)) || isDefined(items.get(selectCtrl.selectValueMap[option.value]));
option.selected = !!value && (includes(value, option.value) ||
includes(value, selectCtrl.selectValueMap[option.value]));
});
};

Expand Down
16 changes: 8 additions & 8 deletions src/ngAnimate/animateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,15 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
return hasMatchingClasses(nA, cR) || hasMatchingClasses(nR, cA);
});

this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap',
this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$Map',
'$$animation', '$$AnimateRunner', '$templateRequest', '$$jqLite', '$$forceReflow',
'$$isDocumentHidden',
function($$rAF, $rootScope, $rootElement, $document, $$HashMap,
function($$rAF, $rootScope, $rootElement, $document, $$Map,
$$animation, $$AnimateRunner, $templateRequest, $$jqLite, $$forceReflow,
$$isDocumentHidden) {

var activeAnimationsLookup = new $$HashMap();
var disabledElementsLookup = new $$HashMap();
var activeAnimationsLookup = new $$Map();
var disabledElementsLookup = new $$Map();
var animationsEnabled = null;

function postDigestTaskFactory() {
Expand Down Expand Up @@ -294,7 +294,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
bool = !disabledElementsLookup.get(node);
} else {
// (element, bool) - Element setter
disabledElementsLookup.put(node, !bool);
disabledElementsLookup.set(node, !bool);
}
}
}
Expand Down Expand Up @@ -597,7 +597,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
animationDetails.runner.end();
/* falls through */
case PRE_DIGEST_STATE:
activeAnimationsLookup.remove(child);
activeAnimationsLookup.delete(child);
break;
}
}
Expand All @@ -607,7 +607,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
function clearElementAnimationState(element) {
var node = getDomNode(element);
node.removeAttribute(NG_ANIMATE_ATTR_NAME);
activeAnimationsLookup.remove(node);
activeAnimationsLookup.delete(node);
}

function isMatchingElement(nodeOrElmA, nodeOrElmB) {
Expand Down Expand Up @@ -717,7 +717,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
var newValue = oldValue
? extend(oldValue, details)
: details;
activeAnimationsLookup.put(node, newValue);
activeAnimationsLookup.set(node, newValue);
}
}];
}];
12 changes: 6 additions & 6 deletions src/ngAnimate/animation.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,21 @@ var $$AnimationProvider = ['$animateProvider', /** @this */ function($animatePro
return element.data(RUNNER_STORAGE_KEY);
}

this.$get = ['$$jqLite', '$rootScope', '$injector', '$$AnimateRunner', '$$HashMap', '$$rAFScheduler',
function($$jqLite, $rootScope, $injector, $$AnimateRunner, $$HashMap, $$rAFScheduler) {
this.$get = ['$$jqLite', '$rootScope', '$injector', '$$AnimateRunner', '$$Map', '$$rAFScheduler',
function($$jqLite, $rootScope, $injector, $$AnimateRunner, $$Map, $$rAFScheduler) {

var animationQueue = [];
var applyAnimationClasses = applyAnimationClassesFactory($$jqLite);

function sortAnimations(animations) {
var tree = { children: [] };
var i, lookup = new $$HashMap();
var i, lookup = new $$Map();

// this is done first beforehand so that the hashmap
// this is done first beforehand so that the map
// is filled with a list of the elements that will be animated
for (i = 0; i < animations.length; i++) {
var animation = animations[i];
lookup.put(animation.domNode, animations[i] = {
lookup.set(animation.domNode, animations[i] = {
domNode: animation.domNode,
fn: animation.fn,
children: []
Expand All @@ -54,7 +54,7 @@ var $$AnimationProvider = ['$animateProvider', /** @this */ function($animatePro

var elementNode = entry.domNode;
var parentNode = elementNode.parentNode;
lookup.put(elementNode, entry);
lookup.set(elementNode, entry);

var parentEntry;
while (parentNode) {
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 @@ -2964,12 +2964,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
2 changes: 1 addition & 1 deletion test/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@

/* apis.js */
"hashKey": false,
"HashMap": false,
"NgMapShim": false,

/* urlUtils.js */
"urlResolve": false,
Expand Down
Loading