Skip to content
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

Collection & Model Mixin #4024

Merged
merged 1 commit into from Apr 18, 2017
Merged

Collection & Model Mixin #4024

merged 1 commit into from Apr 18, 2017

Conversation

megawac
Copy link
Collaborator

@megawac megawac commented Apr 29, 2016

Adds Collection & Model mixins for adding custom implementations of iterator/check methods to base collection and model. The main motivation of this is to add some lodash methods that were missing on the prototype.

Ideally one could do

Backbone.Collection.mixin({
  sum: lodash.sum,
  take: lodash.take
});

However, I don't have a good idea of how to support iteratees, so things like collection.sum('value')

Thoughts? /cc @akre54 @jridgewell

@jridgewell
Copy link
Collaborator

However, I don't have a good idea of how to support iteratees, so things like collection.sum('value')

Hm? You seem to have it already.

@megawac
Copy link
Collaborator Author

megawac commented Apr 30, 2016

Does it? I just assumed it wouldn't get transformed to a (attr) => model.get(attr) as it doesn't hit this case as length != 3 or 4

], function(config) {
var Base = config[0],
methods = config[1],
attribute = config[2];

Choose a reason for hiding this comment

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

Curious to know why you've used an array of array here instead of an array of object here, e.g:

_.each([
   {base:Collection, methods: collectionMethods, attribute: 'models'},
   {base:Model,  methods: modelMethods, attribute: 'attributes'}
], function(config) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No real reason, I don't see a major advantage for either strategy at the moment. Though current impl it lends itself nicely to _.spread if underscore ever decides to follow suit

@jridgewell
Copy link
Collaborator

I just assumed it wouldn't get transformed to a (attr) => model.get(attr) as it doesn't hit this case as length != 3 or 4

Oh, didn't read that case. What fails if we use cb for every case?

@megawac
Copy link
Collaborator Author

megawac commented May 2, 2016

I think _.invoke would fail in addition to omit, difference, union and friends, and perhaps the reduce implementations

@megawac megawac mentioned this pull request May 6, 2016
@rafde
Copy link
Contributor

rafde commented Jan 24, 2017

how has this not been merged yet?

@linus-amg
Copy link
Contributor

linus-amg commented Jan 25, 2017

@rafde maybe you can take a look at backbone-metal, i think its something similar

https://github.com/marionettejs/backbone-metal

@rafde
Copy link
Contributor

rafde commented Jan 25, 2017

this PR is suppose to allow you to define your own functions that would proxy against model.attributes && collection.models

backbone-metal doesn't look like it offers the same feature.

Combine implementation of collection and model underscore methods
@megawac
Copy link
Collaborator Author

megawac commented Apr 9, 2017

Can I get another set of 👀 on this? Want to get it into #4028

@megawac megawac requested a review from akre54 April 9, 2017 01:30
@akre54
Copy link
Collaborator

akre54 commented Apr 18, 2017

Fine by me 👍

@akre54 akre54 merged commit e524804 into jashkenas:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants