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

Very minor re-order of lines in Model constructor #1402

Closed
davidmarble opened this issue Jun 12, 2012 · 4 comments
Closed

Very minor re-order of lines in Model constructor #1402

davidmarble opened this issue Jun 12, 2012 · 4 comments

Comments

@davidmarble
Copy link
Contributor

Currently this.collection is set on a model after parse is called:

var Model = Backbone.Model = function(attributes, options) {
  var defaults;
  attributes || (attributes = {});
  if (options && options.parse) attributes = this.parse(attributes);
  if (defaults = getValue(this, 'defaults')) {
    attributes = _.extend({}, defaults, attributes);
  }
  if (options && options.collection) this.collection = options.collection;

Because parse is such a good place to extend backbone, for example to implement nested collections, and this.collection can be extremely helpful in doing such extending, could you please modify the code order to:

var Model = Backbone.Model = function(attributes, options) {
  var defaults;
  if (options && options.collection) this.collection = options.collection;
  attributes || (attributes = {});
  if (options && options.parse) attributes = this.parse(attributes);
  if (defaults = getValue(this, 'defaults')) {
    attributes = _.extend({}, defaults, attributes);
  }

Simple change, shouldn't affect anything else, and gives parse one extra piece of information to work with.

@braddunbar
Copy link
Collaborator

Heya @davidmarble, thanks for your issue. :)

Could you explain to me why you need this.collection in order to parse nested collections? I don't quite understand your use case.

@davidmarble
Copy link
Contributor Author

Well, I was hoping not to have to add detailed use cases, as this change does nothing functionally to change any current use of backbone. It just allows access to this.collection before parse is called instead of after.

As noted, parse is a good place to extend backbone, for example to implement nested collections. There are many ways people have come up with for doing nested collections on a model. parse is one entry point for doing so -- as an empty, overloadable method called before processing occurs, it's a good place to pre-process nested data. Having access to this.collection in parse means a developer will have access to all properties and methods on the model's collection from within parse.

There are numerous ways this one data member can be used. For example, accessing configuration information stored on this.collection (I'll call this the super-collection) to configure sub-collections a developer wants to nest on one or models within that super-collection.

@braddunbar
Copy link
Collaborator

If you don't have any use case, I'm inclined to leave it as is to eliminate the chance of a regression. Also, I have lots of experience using model.parse for creating nested collections/models and I've never needed to use model.collection. Let me know if you have a good use case for this and we'll reopen it.

@davidmarble
Copy link
Contributor Author

I didn't expect you'd be so quick to dismiss with the explanation already given and the simplicity of what I'm suggesting.

There can't be a regression with this change, The order of these lines is merely a basic code organization decision.

Not every change in a project must have clear use cases. This is a simple change that opens up other opportunities for coders to utilize the framework, takes away no functionality that already exists, and adds no explicit new features that should need defending.

I'll attempt to write up a specific use case in the name of trying hard to give back to the project by encouraging this minor improvement.

An example of code that is impossible to write currently, but would be possible with the above simple change, is commented about under Feed.parse below.

The coffeescript compiled version of the below is here: https://gist.github.com/2922029 (though might seem involved if you're not used to reading coffeescript output). Line 41 of that gist is impossible with the current backbone.js, but is possible with the simple switch above.

Example JSON Data Source - a collection of feeds

[ // Feeds collection of Feed models
  {
    "feed": "Featured News",
    "items": [ // FeedItems collection of FeedItem models
      {
        // FeedItem model
      },
      . . .
    ]
  },
  {
    "feed": "Chronological News",
    "items": [ // FeedItems collection of FeedItem models
      {
        // FeedItem model
      },
      . . .
    ]
  },
  . . .
]

Super-Collection for multiple Feed models from the JSON above

class Feeds extends Collection

  model: Feed
  url: '/feeds/'

  constructor: (models, options) ->
    # Set some instance-specific parameters.
    if options? and options.params
      @params = _.extend({}, options.params)
    else
      @params = {}
    super

Feed Model, which includes FeedItems, a nested Collection of FeedItem models

class Feed extends Model

  constructor: (attrs, options) ->
    # Bool to keep track of whether nested collection models being
    # parsed should be appended or replace the existing models.
    @add_nested = false

    super

  parse: (response, xhr) ->
    # Handle nested feed item collection on the model.
    # Note this only handles one level of nesting, and options that may 
    # have been important on fetch or other methods calling parse()
    # won't be passed up to the embedded `add` and `reset` calls below.

    # If the nested collection hasn't been created, create it.
    # Pass along some options to the constructor.
    # As an example, pass along default parameters from super-collection,
    # replacing some with parameters specific to the nested collection.
    if not @spurs
      opts = 
        # THIS IS NOT POSSIBLE WITH THE CURRENT ORDER OF LINES IN 
        # BACKBONE'S MODEL CONSTRUCTOR. this.collection HAS NOT YET BEEN 
        # SET, AND WON'T BE UNTIL JUST AFTER THIS METHOD COMPLETES.
        params: _.extend(@collection.params, {some_param: overridden})

      # Create the nested collection, passing along options
      @feed_items = new FeedItems(null, opts)

    # Populate @feed_items with response.items, calling `add` or `reset` on the collection
    # and using the collection's `parse` on the items first.
    @feed_items[if @add_nested then 'add' else 'reset'](@feed_items.parse(response.items, xhr))

    # Remove the feed items from the response so we don't have to
    # worry about how they'll be treated in other model methods.
    delete response.items

    return response

Nested Collection on Feed for FeedItem models. With a little work, if the feeds source supported it, this collection could be used on its own to fetch changes to an individual feed.

class FeedItems extends Collection

  model: FeedItem
  url: '/feed/'

Actual feed items

class FeedItem extends Model

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

No branches or pull requests

2 participants