Skip to content

Loading…

Add underscore methods to models #2040

Merged
merged 1 commit into from

8 participants

@akre54
Collaborator

Just like underscore mixins are a huge help for iterating over Collection models (for running filter, pluck, map, etc), give Model attributes some underscore Object methods (really useful for keys, values, omit, and each). I often need to wrap model.attributes to use with underscore functions, and this pull bakes in those methods natively.

@eastridge

Great addition, extremely consistent with the helpers for collection.models. That keys and values aren't already present on Model seems like an omission.

@tgriesser
Collaborator

Great idea. +1

@braddunbar
Collaborator

Seems like it could be handy to me.

@caseywebdev
Collaborator

:+1: Makes sense, why should collections get all the underscore love?

@dgbeck

Very cool idea! On the flip side it opens up a lot of roads to access quasi-private model.attributes whereas now most are nicely funneled through model#get. Maybe just a subset. keys and values would come in handy, but pairs?

@eastridge

Maybe my thinking is flawed here but I think of get as optional, and reading attributes.someKey as just as valid as get('someKey'). Where as set('someKey', 'value') is a required API. I don't think attributes should be renamed but if it's thought of as private by the core team then maybe it really should be _attributes. Since get has no side effects and is thus equivalent to reading directly off of attributes I see no reason no to mixin the underscore methods.

@akre54
Collaborator

@dgbeck I'm not tied to the specific methods (you're right, pairs is weird, but it seemed to fit the m.o. of the other functions).

Other questions: would map be appropriate? Is size a confusing term when called as a method?

@jashkenas
Owner

Brilliant. Is everyone here in agreement with the list of methods proposed in the patch?

@eastridge

I think anything that logically applied to an object in underscore should be applied here including size and map. model.size() is not likely to be that useful but I like the consistency of knowing that everything I can use in underscore on an object is available, just like I know I can use everything available to an array / collection on a Backbone Collection.

@caseywebdev
Collaborator

I agree with @eastridge

@tgriesser
Collaborator

chain would be good to add, other than that it looks good

I wonder if a utility to mix these methods into an object is something that would make sense to add Underscore.

var C = function () { this.models = [] }
var M = function () { this.attributes = {} }

_.makeCollection(C.prototype, 'models')
_.makeModel(M.prototype, 'attributes')

... or something along those lines

@braddunbar
Collaborator

@eastridge You're right, attributes is part of the public api.

@akre54
Collaborator

@tgriesser chain added, @dgbeck pairs removed.

@dgbeck

Got it. Very clean in that case. I always thought of attributes as 'internal' to the model, both read and write. Nice to have that cleared up! Cool addition!

@wyuenho

:+1:

I would certainly use pairs if I want to get to the list of key-value pairs, not that I can't do it now, it's just more consistent. invert is basically the reverse of pairs so they either both stay or both go away.

If you add chain, you should probably add tap too, they also go together.

In essence I agree with @caseywebdev and @eastridge, all the Underscore methods that work on objects should probably be added to Model when appropriate, just don't forget the aliases too.

@tgriesser
Collaborator

I agree that pairs should be on the list - it fits with having any functions which take an object as input. @wyuenho - tap wouldn't be necessary, it only makes sense to use after you start a chain so it wouldn't be called directly on the model.

@wyuenho

Edited: @tgriesser you are right, tap would be of limited utility.

@akre54
Collaborator

@wyuenho @tgriesser you got it. pairs added back in. I'm not really sure of a use case where you'd want this, but it's definitely important from a consistency standpoint.

My only outstanding questions are about the rest of _.collection methods (reduce, find, filter, where, reject, max, et al). Models can ostensibly hold data where these functions are useful (e.g. a sequential list of properties where a BB Collection would be inappropriate), but the use case may not be strong enough for these either.

toArray is essentially an alias for values when used on an Object like Model.attributes. Is this needed?

What could be more useful is are the sortBy and groupBy methods (e.g. I might want to work on a group of attributes by a custom-defined prefix). Will add those in if no objections.

@akre54
Collaborator

@jashkenas: added groupBy and sortBy methods to the pull. OK to merge?

@caseywebdev
Collaborator

@akre54 There's a merge conflict right now, try rebasing.

@akre54
Collaborator

@caseywebdev whups, sorry about that, forgot to pull down from upstream.

@akre54
Collaborator

Can squash these down if that would be helpful too.

@caseywebdev
Collaborator

Yea please squash and I'll merge.

@caseywebdev caseywebdev merged commit ab4a794 into jashkenas:master

1 check passed

Details default The Travis build passed
@caseywebdev
Collaborator

Thanks @akre54!

@jashkenas
Owner

Whoops -- I think this needs a bit more discussion before a merge. There's no way to "reopen" a pull request, is there?

@caseywebdev
Collaborator

Discussion regarding the addition of underscore methods or which methods specifically? And I don't think it can be reopened.

@akre54
Collaborator

@jashkenas want to revert the commit and I'll reopen the pull open a new pull?

@caseywebdev
Collaborator

The implementation looks fine to me, it would just be the underscore methods (if any) that would need to be changed right?

@jashkenas
Owner

I'm concerned that they'll be more confusing than helpful for the average Backbone model. Underscore collection functions usually operate on arrays of similar values, or objects where all of the properties are similar values. Your basic model, with attributes that all represent different columns in a database, isn't something you'd normally want to contains, include, or map, etc.

That said, one of your attributes may be an array that you do want to do those things to.

A revert plus a reopen would be lovely.

@caseywebdev
Collaborator

@jashkenas reverted, my bad!

@akre54
Collaborator

@jashkenas @caseywebdev hrrm... github is complaining that master is already up to date with my branch....

@caseywebdev
Collaborator

You'll have to create a new commit, since the commit in this PR is already in master.

@akre54
Collaborator

ok, I'll cherry-pick. hold on

@tgriesser
Collaborator

@akre54 if you add an upstream remote and fetch from that, you'll get the latest. See step 3

@akre54
Collaborator
@akre54 akre54 deleted the akre54:add-underscore-methods-to-model branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 6, 2013
  1. @akre54

    Add keys, values, pairs, invert, pick, omit, each, and other

    akre54 committed
    underscore methods to models
Showing with 45 additions and 0 deletions.
  1. +14 −0 backbone.js
  2. +31 −0 test/model.js
View
14 backbone.js
@@ -544,6 +544,20 @@
});
+ // Underscore methods that we want to implement on the Model.
+ var methods = ['each', 'forEach', 'map', 'collect', 'contains', 'include',
+ 'sortBy', 'groupBy', 'size', 'keys', 'values', 'pairs', 'invert', 'pick',
+ 'omit', 'isEqual', 'isEmpty', 'chain'];
+
+ // Mix in each Underscore method as a proxy to `Model#attributes`.
+ _.each(methods, function(method) {
+ Model.prototype[method] = function() {
+ var args = slice.call(arguments);
+ args.unshift(this.attributes);
+ return _[method].apply(_, args);
+ };
+ });
+
// Backbone.Collection
// -------------------
View
31 test/model.js
@@ -111,6 +111,37 @@ $(document).ready(function() {
equal(model.url(), '/nested/1/collection/2');
});
+ test("underscore methods", 19, function() {
+ var model = new Backbone.Model({ 'foo': 'a', 'bar': 'b', 'baz': 'c' });
+ var model2 = model.clone();
+ deepEqual(model.keys(), ['foo', 'bar', 'baz']);
+ deepEqual(model.values(), ['a', 'b', 'c']);
+ deepEqual(model.pairs(), [['foo', 'a'], ['bar', 'b'], ['baz', 'c']]);
+ deepEqual(model.invert(), { 'a': 'foo', 'b': 'bar', 'c': 'baz' });
+ deepEqual(model.pick('foo', 'baz'), {'foo': 'a', 'baz': 'c'});
+ deepEqual(model.omit('foo', 'bar'), {'baz': 'c'});
+ model.each(function(attr, index, attrs) {
+ ok(true);
+ });
+ equal(model.isEqual(model2.attributes), true);
+ equal(model.isEqual({ 'foo': 'a', 'bop': 'd' }), false);
+ equal(model.isEmpty(), false);
+ equal(model.size(), 3);
+ equal(model.contains('foo'), false);
+ equal(model.contains('b'), true);
+ equal(model.chain().keys().contains('foo').value(), true);
+ deepEqual(model.sortBy(function(val) { return val === 'b' }), ["c", "a", "b"]);
+ deepEqual(model.groupBy(), {
+ 'a': ['a'],
+ 'b': ['b'],
+ 'c': ['c']
+ });
+ var prefixed = model.map(function(val, key, attrs){
+ return 'book-' + key;
+ });
+ deepEqual(prefixed, ['book-foo', 'book-bar', 'book-baz']);
+ });
+
test("clone", 10, function() {
var a = new Backbone.Model({ 'foo': 1, 'bar': 2, 'baz': 3});
var b = a.clone();
Something went wrong with that request. Please try again.