feat(limitTo): add support for array-likes #14694

wesleycho · 2016-05-31T04:59:43Z

Add support for array-like objects

This implements #14657.

campersau · 2016-05-31T07:28:15Z

test/ng/filter/limitToSpec.js

    limitTo = $filter('limitTo');
  }));

+  function isInObject(obj) {


This is never used.

gkalpak · 2016-05-31T09:00:30Z

src/ng/filter/limitTo.js

      }
    }
  };
 }
+
+function sliceFn(input, idx, length) {
+  if (isString(input)) return input.slice(idx, length);


I would check if input has a slice method (or property) so we support Array-like objects with custon slice methods.

This separation is specifically because String.prototype.slice returns a string, while Array.prototype.slice returns an array.

OK, let's stick with the current separation, so we are always returnign either a string or an array.

One alternative implementation I can do is instead of the isString(input) check, I can do

return input.slice ? input.slice.call(input, idx, length) : slice.call(input, idx, length);

Thoughts?

Nevermind, looks like this fails on falsy types.

I think it's best to keep things as is, so we don't break the guarantee that limitTo returns an Array or String.
(I don't see any real benefit in breaking that contract.)

gkalpak · 2016-05-31T09:12:39Z

Generally looks good - I left some comments.
I think it would be nice to test against a couple more Array-likes (e.g. a NodeList, a jqLite/jQuery collection).

wesleycho · 2016-05-31T11:31:20Z

Made adjustments as per comments.

I do have a question though - I notice in the orderBy filter, it only checks against a similar object I have constructed here, and not against typical alternative array-likes. Does it make sense to test those here given that?

gkalpak · 2016-05-31T12:47:13Z

test/ng/filter/limitToSpec.js

  });

  it('should return an empty string when X = 0', function() {
    expect(limitTo(str, 0)).toEqual("");
    expect(limitTo(str, '0')).toEqual("");
+    expect(limitTo({}, 1)).toEqual({});


gkalpak · 2016-05-31T12:51:25Z

$filter has a couple of array-likes (see filterSpec.js#L428-L443), but I don't think consistency is important here.

I don't mean add all array-likes in all usecases, just add a simple testcase that tests basic limiting on 2-3 array-like types. But I don't feel strongly about it, so feel free to ignore 😄

wesleycho · 2016-05-31T20:59:56Z

Updated as per comments (sloppiest PR I've done in a while...) - I also added a simple test case for basic array likes at the very end.

wesleycho · 2016-06-01T06:10:47Z

Bah, I did a mistaken force push to try to restart the CI - will push again when I'm at work tomorrow, some changes got wiped out.

wesleycho · 2016-06-07T16:53:27Z

This should be up to date now.

gkalpak · 2016-06-07T20:29:58Z

src/ng/filter/limitTo.js

      }
    }
  };
 }
+
+function sliceFn(input, idx, length) {


Nitpick: Renaming the arguments to input, begin, end sounds more intuitive.

gkalpak · 2016-06-07T20:34:34Z

test/ng/filter/limitToSpec.js

    expect(limitTo(null, 1)).toEqual(null);
    expect(limitTo(undefined, 1)).toEqual(undefined);
-    expect(limitTo({}, 1)).toEqual({});


gkalpak · 2016-06-07T20:37:40Z

test/ng/filter/limitToSpec.js

+
+    expect(limitTo(argsObj, 2).length).toBe(2);
+    expect(limitTo('abc', 1).length).toBe(1);
+    expect(limitTo(nodeList, 3).length).toBe(3);


This is not testing anything. You need a limit that is less than the actual length.

gkalpak · 2016-06-07T20:41:47Z

Now it's time for the "real stuff": We need to update the docs 😃

wesleycho · 2016-06-07T20:53:35Z

Added documentation update, might need tweaking on wording (or maybe removing the bit about strings or numbers).

gkalpak · 2016-06-08T21:47:54Z

@wesleycho, I don't see any docs 😟

- Add support for array-like objects

wesleycho · 2016-06-08T21:56:16Z

That's because I'm a moron :( - pushed

petebacondarwin · 2016-06-10T09:55:56Z

src/ng/filter/limitTo.js

 *
- * @param {Array|string|number} input Source array, string or number to be limited.
+ * @param {Array} input Array or array-like to be limited.


So the param type should be {Array|string|number|object} right?

In orderBy, we use ArrayLike (which is totally made-up, but pretty descriptive).

OK. That works for me - does orderBy convert numbers to strings too?

orderBy only accepts {Array|ArrayLike} (and calls Array.prototype.map on its input).

gkalpak · 2016-06-10T11:39:06Z

I just realized, @googlebot didn't bother verifying the CLA fos us 😒
Can we trigger it manually ?

petebacondarwin · 2016-06-10T12:28:39Z

Not sure but @wesleycho is definitely a yes already :-)

gkalpak · 2016-06-10T12:31:41Z

I'll add the label manually, because my PR-merging script doesn't proceed otherwise :)

Fixes #14657 Closes #14694

campersau reviewed May 31, 2016
View reviewed changes

test/ng/filter/limitToSpec.js

limitTo = $filter('limitTo');

}));

function isInObject(obj) {

Copy link

Contributor

campersau May 31, 2016

Choose a reason for hiding this comment

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

This is never used.

gkalpak added type: feature frequency: low severity: inconvenient component: filters labels May 31, 2016

gkalpak added this to the Backlog milestone May 31, 2016

gkalpak reviewed May 31, 2016
View reviewed changes

gkalpak added the needs: work label May 31, 2016

wesleycho force-pushed the feat/limit-arraylike branch from 26caa6f to 12f0a49 Compare May 31, 2016 11:29

wesleycho force-pushed the feat/limit-arraylike branch 2 times, most recently from 87212f9 to 50e46a3 Compare May 31, 2016 11:54

gkalpak reviewed May 31, 2016
View reviewed changes

wesleycho force-pushed the feat/limit-arraylike branch 2 times, most recently from 372d5f3 to cb5f3fa Compare May 31, 2016 20:59

wesleycho force-pushed the feat/limit-arraylike branch from cb5f3fa to 372d5f3 Compare June 1, 2016 06:09

wesleycho force-pushed the feat/limit-arraylike branch from 372d5f3 to cb5f3fa Compare June 2, 2016 18:52

gkalpak reviewed Jun 7, 2016
View reviewed changes

wesleycho force-pushed the feat/limit-arraylike branch from cb5f3fa to 48c02f3 Compare June 7, 2016 20:32

gkalpak reviewed Jun 7, 2016
View reviewed changes

wesleycho force-pushed the feat/limit-arraylike branch from 48c02f3 to 84308a4 Compare June 7, 2016 20:36

gkalpak reviewed Jun 7, 2016
View reviewed changes

wesleycho force-pushed the feat/limit-arraylike branch from 84308a4 to 9b9d536 Compare June 7, 2016 20:39

wesleycho force-pushed the feat/limit-arraylike branch from 9b9d536 to 58e0e4e Compare June 7, 2016 20:53

feat(limitTo): add support for array-likes

8905747

- Add support for array-like objects

wesleycho force-pushed the feat/limit-arraylike branch from 58e0e4e to 8905747 Compare June 8, 2016 21:56

petebacondarwin reviewed Jun 10, 2016
View reviewed changes

gkalpak added the cla: yes label Jun 10, 2016

gkalpak closed this in f467dc3 Jun 10, 2016

gkalpak pushed a commit that referenced this pull request Jun 10, 2016

feat(limitTo): add support for array-like objects

b03957f

Fixes #14657 Closes #14694

wesleycho deleted the feat/limit-arraylike branch June 10, 2016 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(limitTo): add support for array-likes #14694

feat(limitTo): add support for array-likes #14694

wesleycho commented May 31, 2016

campersau May 31, 2016

gkalpak May 31, 2016

wesleycho May 31, 2016

gkalpak May 31, 2016

wesleycho May 31, 2016

wesleycho May 31, 2016

gkalpak Jun 1, 2016

gkalpak commented May 31, 2016 •

edited

Loading

wesleycho commented May 31, 2016

gkalpak May 31, 2016

gkalpak commented May 31, 2016

wesleycho commented May 31, 2016

wesleycho commented Jun 1, 2016

wesleycho commented Jun 7, 2016

gkalpak Jun 7, 2016

gkalpak Jun 7, 2016

gkalpak Jun 7, 2016

gkalpak commented Jun 7, 2016

wesleycho commented Jun 7, 2016

gkalpak commented Jun 8, 2016

wesleycho commented Jun 8, 2016

petebacondarwin Jun 10, 2016

gkalpak Jun 10, 2016

petebacondarwin Jun 10, 2016

gkalpak Jun 10, 2016

gkalpak commented Jun 10, 2016

petebacondarwin commented Jun 10, 2016

gkalpak commented Jun 10, 2016

feat(limitTo): add support for array-likes #14694

feat(limitTo): add support for array-likes #14694

Conversation

wesleycho commented May 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented May 31, 2016 • edited Loading

wesleycho commented May 31, 2016

Choose a reason for hiding this comment

gkalpak commented May 31, 2016

wesleycho commented May 31, 2016

wesleycho commented Jun 1, 2016

wesleycho commented Jun 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jun 7, 2016

wesleycho commented Jun 7, 2016

gkalpak commented Jun 8, 2016

wesleycho commented Jun 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jun 10, 2016

petebacondarwin commented Jun 10, 2016

gkalpak commented Jun 10, 2016

gkalpak commented May 31, 2016 •

edited

Loading