New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate error when attempting to use filter on objects #9992

Closed
justincy opened this Issue Nov 10, 2014 · 11 comments

Comments

@justincy
Contributor

justincy commented Nov 10, 2014

Currently it fails silently if you try to use filter on objects, as in the example below, because it only supports arrays.

<div ng-repeat="item in object | filter:{display:true}">stuff</div>

I blew a ton of time on this because it's not very intuitive since ng-repeat supports objects. I fully expect to make the same mistake again. A simple error message would save a world of hurt.

@BrianMinister

This comment has been minimized.

Show comment
Hide comment
@BrianMinister

BrianMinister Nov 11, 2014

I am concerned that if angular performs an is array check on every ng-repeat directive, one of the slowest directives in angular may get notably slower on more complex dashboard applications like mine where I have 2 or 3 layers of nested ng-repeats.

BrianMinister commented Nov 11, 2014

I am concerned that if angular performs an is array check on every ng-repeat directive, one of the slowest directives in angular may get notably slower on more complex dashboard applications like mine where I have 2 or 3 layers of nested ng-repeats.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 11, 2014

Contributor

I am concerned that if angular performs an is array check on every ng-repeat directive, one of the slowest directives in angular may get notably slower on more complex dashboard applications like mine where I have 2 or 3 layers of nested ng-repeats.

this is not likely to matter at all --- it's a one-time operation and it's a very small slice of the time (per set, as opposed to per item) taken to process that array in the first place. Even for nested repeats, the work isn't going to grow exponentially, it's really not going to matter

Contributor

caitp commented Nov 11, 2014

I am concerned that if angular performs an is array check on every ng-repeat directive, one of the slowest directives in angular may get notably slower on more complex dashboard applications like mine where I have 2 or 3 layers of nested ng-repeats.

this is not likely to matter at all --- it's a one-time operation and it's a very small slice of the time (per set, as opposed to per item) taken to process that array in the first place. Even for nested repeats, the work isn't going to grow exponentially, it's really not going to matter

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 11, 2014

Contributor

@justincy would you be interested in sending a PR for this? it should be a pretty trivial 1 line fix + a test case

Contributor

caitp commented Nov 11, 2014

@justincy would you be interested in sending a PR for this? it should be a pretty trivial 1 line fix + a test case

@justincy

This comment has been minimized.

Show comment
Hide comment
@justincy

justincy Nov 11, 2014

Contributor

@caitp I am interested but I've never looked at the angular.js source code so I can't promise anything.

Contributor

justincy commented Nov 11, 2014

@caitp I am interested but I've never looked at the angular.js source code so I can't promise anything.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 11, 2014

Contributor

If I'm not mistaken, it's basically this:

function filterFilter() {
  return function(array, expression, comparator) {
-   if (!isArray(array)) return array;
+   if (!isArray(array)) throw $minErr('filter')('notarray', 'expected Array but received {0}'', array);

    var comparatorType = typeof(comparator),
        predicates = [];

(and then you'd need to make up a 'notarray' error document in docs/content/error/filter/notarray.ngdoc --- but its not hard.

Contributor

caitp commented Nov 11, 2014

If I'm not mistaken, it's basically this:

function filterFilter() {
  return function(array, expression, comparator) {
-   if (!isArray(array)) return array;
+   if (!isArray(array)) throw $minErr('filter')('notarray', 'expected Array but received {0}'', array);

    var comparatorType = typeof(comparator),
        predicates = [];

(and then you'd need to make up a 'notarray' error document in docs/content/error/filter/notarray.ngdoc --- but its not hard.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 11, 2014

Contributor

then again, I guess this would be a breaking change, so really you'd just want to log it --- even simpler

Contributor

caitp commented Nov 11, 2014

then again, I guess this would be a breaking change, so really you'd just want to log it --- even simpler

@caitp caitp closed this Nov 11, 2014

@caitp caitp reopened this Nov 11, 2014

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 11, 2014

Contributor

(oops, too much coffee)

Contributor

caitp commented Nov 11, 2014

(oops, too much coffee)

@IShotTheSheriff

This comment has been minimized.

Show comment
Hide comment
@IShotTheSheriff

IShotTheSheriff Nov 13, 2014

Contributor

Hi, I would be also interested in this issue, as it seems like a good place to start contributing adventure ;). I've started to work on that, but I couldn't make the tests passes. Could I ask for one more tip about how to use logging inside your source files? I've done it like this:

filterFilter.$inject = ['$log'];
function filterFilter($log) {
  return function(array, expression, comparator) {
   if (!isArray(array)) { 
     $log.error(new Error('filter expcted array but received ' + typeof array))
     return array;
   };

I've tested the solution and I've added unit test and it passes. Unfortunately from ngMock unit tests I get error:

Expected $log to be empty! Either a message was logged unexpectedly, or an expected log message was not checked and removed.

If it wouldn't be a problem could you please point me gently to the right direction with this one?

Contributor

IShotTheSheriff commented Nov 13, 2014

Hi, I would be also interested in this issue, as it seems like a good place to start contributing adventure ;). I've started to work on that, but I couldn't make the tests passes. Could I ask for one more tip about how to use logging inside your source files? I've done it like this:

filterFilter.$inject = ['$log'];
function filterFilter($log) {
  return function(array, expression, comparator) {
   if (!isArray(array)) { 
     $log.error(new Error('filter expcted array but received ' + typeof array))
     return array;
   };

I've tested the solution and I've added unit test and it passes. Unfortunately from ngMock unit tests I get error:

Expected $log to be empty! Either a message was logged unexpectedly, or an expected log message was not checked and removed.

If it wouldn't be a problem could you please point me gently to the right direction with this one?

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Nov 22, 2014

Member

I think that we should throw, but because that's a breaking change we should schedule this for 1.4

Member

IgorMinar commented Nov 22, 2014

I think that we should throw, but because that's a breaking change we should schedule this for 1.4

@Puigcerber

This comment has been minimized.

Show comment
Hide comment
@Puigcerber

Puigcerber Dec 6, 2014

Contributor

While creating this pull request I've noticed there are two protractor tests failing. The should auto compile for the example-example51 throws Expected 'name}}!' to be 'Angular!'. but is not related to these changes and fails for me as well in master. Could somebody try it out?

Contributor

Puigcerber commented Dec 6, 2014

While creating this pull request I've noticed there are two protractor tests failing. The should auto compile for the example-example51 throws Expected 'name}}!' to be 'Angular!'. but is not related to these changes and fails for me as well in master. Could somebody try it out?

@mgcrea

This comment has been minimized.

Show comment
Hide comment
@mgcrea

mgcrea Jul 13, 2015

Contributor

This type check broke promise usage with filters, the following expression is now throwing errors before resolving the promise:

getAddress($viewValue) | filter:"Al" | limitTo:6
  $scope.getAddress = function() {
    return $q(function(resolve, reject) {
      resolve(['Alabama', 'Alaska', ...]);
    });
  }

This broke AngularStrap typeahead with remote source example.

Is there another way to achieve this?
If not, I think we should revert this & remove the check/error (or at least allow promises).

Contributor

mgcrea commented Jul 13, 2015

This type check broke promise usage with filters, the following expression is now throwing errors before resolving the promise:

getAddress($viewValue) | filter:"Al" | limitTo:6
  $scope.getAddress = function() {
    return $q(function(resolve, reject) {
      resolve(['Alabama', 'Alaska', ...]);
    });
  }

This broke AngularStrap typeahead with remote source example.

Is there another way to achieve this?
If not, I think we should revert this & remove the check/error (or at least allow promises).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment