'list.filter is not a function' in filterFromRegistry in animateQueue.js in 1.5.5+

[kyrylkov]

Note: for support questions, please use one of these channels: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#question. This repository's issues are reserved for feature requests and bug reports.

Do you want to request a feature or report a bug?
bug

What is the current behavior?
error: list.filter is not a function

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (template: http://plnkr.co/edit/tpl:yBpEi4).
call $animate.off(element);

What is the expected behavior?
works in 1.5.3 and before

What is the motivation / use case for changing the behavior?
bug introduced in 1.5.5 when filterFromRegistry was changed from a being local function of $animate.off method in animateQueue.js

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.
1.5.5 and later, all browsers and OSes. Works with 1.5.3 and before. Still broken in master branch.

Other information (e.g. stacktraces, related issues, suggestions how to fix)
function filterFromRegistry(list, matchContainer, matchCallback) takes three parameters, but is called with two only in callbackRegistry[eventType] = filterFromRegistry(callbackRegistry[eventType], container); causing TypeError list.filter is not a function

[gkalpak]

I don't see why this would be a problem. Omitting the third parameter (callback), doesn't explain the list.filter is not a function error. Could you create a demo (e.g. using CodePen, Plnkr, etc) ?

[kyrylkov]

Broken by ea4120b

[gkalpak]

This could happen under some circumstances if callbackRegistry[eventType] === null.
We should probably short-circuit this case in filterFromRegistry:

function filterFromRegistry(list, matchContainer, matchCallback) {
  if (!list) return list;
  ...
}

cc @Narretz

[kyrylkov]

@gkalpak @Narretz

function filterFromRegistry(list, matchContainer, matchCallback) {
  console.log(typeof list);
  console.log(list);
  ...
}

shows function, so no, in our case it's not null

The function body is:

function() {
  for (var i in this) {
    if (this[i] === true) {
      return false;
    }
  }
  return true;
}

which is defined in our code as:

Object.prototype.allFalse = function() {
  for (let i in this) {
    if (this[i] === true) {
      return false;
    }
  }
  return true;
};

and used to disable a button if none of the checkboxes are selected:

<button ... ng-disabled="$ctrl.importing || $ctrl.importTypes.allFalse()" ...>
  ...
</button>

[kyrylkov]

@gkalpak @Narretz

We fixed the code we work with by moving allFalse from Object.prototype to a regular function.

However it did work in 1.5.3 and before and stopped working after ea4120b

[Narretz]

@gkalpak Since you are assigned, do you want to write a fix for this?

[gkalpak]

@Narretz, as far as I understand from @kyrylkov's comments, the reason this breaks is because they added custom, enumerable properties on Object.prototype. If people do this, lots of other stuff could break. Do you still think we should "fix" this?

We could use Object.keys(...).forEach(...) instead of for ... in ... in animateQueue.js#L246.

[Narretz]

Could we also use callbackRegistry = createMap()?

[gkalpak]

Probably 😃