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

filterFilter doesn't work with Array like objects anymore #11782

Closed
superasn opened this Issue May 1, 2015 · 8 comments

Comments

@superasn

superasn commented May 1, 2015

filterFilter also doesn't work with Array like objects anymore, like it used to before.

Please take a look at this fiddle:

Works like it should with previous version of Angular (v1.2):
https://jsfiddle.net/superasn/gznzjgmb/

Breaks with newer versions of Angular (v1.3 and above):
https://jsfiddle.net/superasn/c15yvq6g/

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin May 1, 2015

Member

@gkalpak can you take a look at this one?

Member

petebacondarwin commented May 1, 2015

@gkalpak can you take a look at this one?

gonzaloruizdevilla pushed a commit to gonzaloruizdevilla/angular.js that referenced this issue May 1, 2015

Gonzalo Ruiz de Villa
fix(filterFilter): allow array like objects to be filtered
Throw error if filter is not used with an array like object.

Closes #11782
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak May 1, 2015

Member

Will do.

Member

gkalpak commented May 1, 2015

Will do.

gonzaloruizdevilla pushed a commit to gonzaloruizdevilla/angular.js that referenced this issue May 1, 2015

Gonzalo Ruiz de Villa
fix(filterFilter): allow array like objects to be filtered
Throw error if filter is not used with an array like object.

Closes #11782
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak May 2, 2015

Member

A little background:

  • This was/is not working on 1.2.x. In fact, "array-like" objects have not been supported since v1.1.3.
  • In this context, "array-like" objects are objects prototypally inheriting from Array (so not what Angular internally refers to as "array-like" (i.e. objects for which isArrayLike() returns true)).
  • Pre v1.1.3, filterFilter was checking whether array instanceof Array (which included "subclasses" of Array). Then is switched to Object.prototype.toString.call(array) === '[object Array]' and later to Array.isArray(array).

That said, I don't see why we couldn't support array-like objects (in the internal Angular sense). It seems to be a trivial change.

@gonzaloruizdevilla has already submitted a PR (#11787) we can work on, so let's move the discussion there.

Member

gkalpak commented May 2, 2015

A little background:

  • This was/is not working on 1.2.x. In fact, "array-like" objects have not been supported since v1.1.3.
  • In this context, "array-like" objects are objects prototypally inheriting from Array (so not what Angular internally refers to as "array-like" (i.e. objects for which isArrayLike() returns true)).
  • Pre v1.1.3, filterFilter was checking whether array instanceof Array (which included "subclasses" of Array). Then is switched to Object.prototype.toString.call(array) === '[object Array]' and later to Array.isArray(array).

That said, I don't see why we couldn't support array-like objects (in the internal Angular sense). It seems to be a trivial change.

@gonzaloruizdevilla has already submitted a PR (#11787) we can work on, so let's move the discussion there.

@gkalpak gkalpak self-assigned this May 2, 2015

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin May 2, 2015

Member

One can ducktype an array for one's own purposes without it being a proper subclass and still like Angular to treat it like a collection in some cases.

Member

petebacondarwin commented May 2, 2015

One can ducktype an array for one's own purposes without it being a proper subclass and still like Angular to treat it like a collection in some cases.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp May 2, 2015

Contributor

https://jsfiddle.net/0L11e3xf/ here's a proper version of the 1.3 repro posted above which works correctly, by subclassing the array "properly" --- but yes, filterFilter should replace isArray() with isArrayLike(), I guess. It would be a bonus if the isArrayLike() test didn't use the hasProperty() check :|

Contributor

caitp commented May 2, 2015

https://jsfiddle.net/0L11e3xf/ here's a proper version of the 1.3 repro posted above which works correctly, by subclassing the array "properly" --- but yes, filterFilter should replace isArray() with isArrayLike(), I guess. It would be a bonus if the isArrayLike() test didn't use the hasProperty() check :|

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak May 2, 2015

Member

@caitp, still not sure what you suggest :)

This doesn't work as is (because of the isArray(array) check and array.filter).
I suggested replacing isArray with isArrayLike and array.filter(...) with isArray(array) ? array.filter(...) : Array.prototype.filter.call(array, ...).

Are you saying something different ?

Regarding the length > 0 && (length - 1) in obj check in isArrayLike, I agree it is useless, but doesn't hurt either (other than slowing it down). I would be fine removing it.

Member

gkalpak commented May 2, 2015

@caitp, still not sure what you suggest :)

This doesn't work as is (because of the isArray(array) check and array.filter).
I suggested replacing isArray with isArrayLike and array.filter(...) with isArray(array) ? array.filter(...) : Array.prototype.filter.call(array, ...).

Are you saying something different ?

Regarding the length > 0 && (length - 1) in obj check in isArrayLike, I agree it is useless, but doesn't hurt either (other than slowing it down). I would be fine removing it.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp May 2, 2015

Contributor

but yes, filterFilter should replace isArray() with isArrayLike(), I guess.

I had to re-word that whole slew of things to sound less negative, that's all

Contributor

caitp commented May 2, 2015

but yes, filterFilter should replace isArray() with isArrayLike(), I guess.

I had to re-word that whole slew of things to sound less negative, that's all

@gonzaloruizdevilla

This comment has been minimized.

Show comment
Hide comment
@gonzaloruizdevilla

gonzaloruizdevilla May 2, 2015

Contributor

@gkalpak I don't think the typeof length === 'number' && length > 0 && (length - 1) in obj part is useless.
Actually, my PR #1938 with the first isArrayLike implementation was meant to distinguish between arrayLike objects and other objects with a numeric length property.

It's funny to see how today's implementation is so similar to the one I PR those days. @IgorMinar had to make a difficult implementation due to IE8 support in those versions (ec54712)

Contributor

gonzaloruizdevilla commented May 2, 2015

@gkalpak I don't think the typeof length === 'number' && length > 0 && (length - 1) in obj part is useless.
Actually, my PR #1938 with the first isArrayLike implementation was meant to distinguish between arrayLike objects and other objects with a numeric length property.

It's funny to see how today's implementation is so similar to the one I PR those days. @IgorMinar had to make a difficult implementation due to IE8 support in those versions (ec54712)

gonzaloruizdevilla pushed a commit to gonzaloruizdevilla/angular.js that referenced this issue May 16, 2015

Gonzalo Ruiz de Villa
fix(filterFilter): allow array like objects to be filtered
Throw error if filter is not used with an array like object.

Closes #11782

gonzaloruizdevilla pushed a commit to gonzaloruizdevilla/angular.js that referenced this issue May 16, 2015

Gonzalo Ruiz de Villa
fix(filterFilter): allow array like objects to be filtered
Throw error if filter is not used with an array like object.

Closes #11782

gonzaloruizdevilla pushed a commit to gonzaloruizdevilla/angular.js that referenced this issue May 19, 2015

Gonzalo Ruiz de Villa
feat(filterFilter): allow array like objects to be filtered
Throw error if filter is not used with an array like object.

Closes #11782

@gkalpak gkalpak closed this in 1b0d0fd May 19, 2015

netman92 added a commit to netman92/angular.js that referenced this issue Aug 8, 2015

feat(filterFilter): allow array like objects to be filtered
Throw error if filter is not used with an array like object.

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