Skip to content

saving model and collection options #2458

Closed
jstrimpel opened this Issue Apr 4, 2013 · 36 comments
@jstrimpel

Is there a reason why model and collection do not save off a copy of the options hash to the instance like a view? I see key properties getting plucked in the source and added to the instance, but then the options argument is not assigned to .options.

@jashkenas
Owner

Yep, because unlike a view, there's no standard behavior tagged in the options. If you want to attach them as a property, feel free to do it yourself in your initialize.

Also, please search before opening tickets. This has been asked before.

@jashkenas jashkenas closed this Apr 4, 2013
@jstrimpel

Sorry I did not see that someone had already posed the question though I assumed it had been asked. I admit my search was not very thorough.

Would changing models and collections to set this.options upon construction be a breaking change? Is there any harm in having another reference to the options argument in the model or collection instance? I have had to code around this behavior now and in the past when developing APIs on top of Backbone. While this is not a major issue for me I can see how it might be confusing to developers who are newer to Backbone and have been primarily working in views where they do not have to explicitly create a reference to the options argument in their initialize method or override the constructor.

@braddunbar
Collaborator

It would probably be best if options attachment was left up to the user in Backbone.View as well, but I think that ship has sailed. It's the kind of decision that shouldn't be made for the user, especially since it's so easy to do yourself. There are several pretty good discussions on it in old issues.

@jstrimpel

I agree. I do not like it when decisions are made for me either, but since it is already being done for a developer in the view it makes sense to me to be consistent. It is pretty common practice to keep a reference to an options object as well - most jQuery plugins do this - and as a developer I would expect it based on the behavior of other libraries including Backbone views. However, at this point developers likely have initialize functions setting it and may be massaging the data before setting it, so making a change like this might cause people some issues. It just becomes a bit of pain for me when I have factories that are creating child models and collections based on a complex parent model. The relationship between these objects relies on options being passed to the parent upon construction. I end up having to override constructors just so that I can set the options because I need access to them later in custom sync functions. Not a huge deal, but it seems unnecessary and I thought maybe others were having to do this frequently.

@fredsterss

+1. It's inconsistent and confusing that this is done automatically in Backbone.View and not Model or Collection.

@there4
there4 commented Apr 5, 2013

+1 I just spent time going through the annotated source trying to figure out why my collection options weren't being kept. At the very least, it would be useful to have a note in the documentation explaining the inconsistency.

@pknepper
pknepper commented Apr 5, 2013

+1

@jashkenas jashkenas reopened this Apr 6, 2013
@caseywebdev
Collaborator

If we're going for consistency, I'd like to see options-instance attachment removed from views, but it may be too late for that...

@jashkenas
Owner

If both you and Brad feel that way, maybe we should just bite the bullet and do it with a line in the next "upgrading" paragraph about it...

@caseywebdev
Collaborator

It just feels "wrong"? It's super easy to do that to all of your Models/Views/Collections/Routers/Whatever if you want to, but if we do it for you, you can't opt out. I always feel weird when I have this.collection and this.options.collection, but not this.something and this.options.something.

@braddunbar
Collaborator

Yep, attaching options always leads to a mess where you're not sure which property to use, this.foo or this.options.foo. Further, if you update one you need to update the other or you have a state mismatch. I've seen code where the options aren't attached at all and this.options.foo is used everywhere but this seems inelegant at best.

Options are a vehicle for getting information into your constructor without needing more arguments. Deal with them there, and throw them away.

@there4
there4 commented Apr 6, 2013

It sounds like it should be removed from the view to create consistency. Anybody know if any of the Backbone layout managers and extensions are using it?

@philfreo
philfreo commented Apr 6, 2013

It sounds like it should be removed from the view to create consistency. Anybody know if any of the Backbone layout managers and extensions are using it?

Lots of projects use this, you can be sure. Whether it should be removed I don't care too much either way, but it would definitely require code changes by many, so there should be a good reason for the backwards incompatibility.

@jstrimpel

@braddunbar agreed. this.foo vs. this.options.foo can get confusing and state management is a pain unless the values are arrays or objects. :) I also agree about options being an argument that gets thrown away after construction. I typically pick the properties I need and assign them to this. However, having to override a constructor or call super__.initialize when extending classes when I do not know the inheritance chain - I have to support 40+ developers - just to get the options is unfortunate, but this might be unique to my situation.

My only complaint is that the inconsistency is confusing especially for those new to Backbone. Also, I am sure this in the docs, but when I first started using Backbone the fact that Backbone picks out certain properties out by key name and assigns them to this was initially confusing. None of these things are issues once you start combing through the source, but I am assuming that most new users don't consult the source.

I think removing options from views at this point would be painful for developers at this point. -1

@wookiehangover
Collaborator

+1 for dropping this.options. It has always stood out to me as an unexplained inconsistency.

Even if large apps are depending on this.options, it's a one-liner to add the reference back in to your views (well, 2 if you count adding the named argument, but the point is that it's a simple change for anyone to make.)

@xtal0
xtal0 commented Apr 8, 2013

+1

@dgbeck
dgbeck commented Apr 8, 2013

Agree with removing in principle, but obviously this is a significant change and Backbone has a huge user base. Also, it seems like there the root of the issue may go deeper. There has also been a lot of confusion / discussion is the past around why some options are attached directly to the view and others are not :

There are several special options that, if passed, will be attached directly to the view: model, collection, el, id, className, tagName and attributes.

As long as we are considering a significant breaking change it might be worth to take a step back and see if there is a restructuring that might address both issues.

Ideally you could "automagically" attach the options that you want to the view object itself, right? As discussed in #2069, there are several ways to go about "white listing" options to attach to the view. Interesting though, is that, at least in our experience, you almost always want to attach all the options to the view. It is a pity that people are forced to manually copy in each view class (or develop their own solution in a base view class) just because of the fringe cases where you don't want to attach the options to the view.

The source of this problem may be that there are three distinct things that are being squished into the options parameter - the built in options that get attached directly to the view, user defined options that you want to attach directly to the view, and then temporary options that you just need during initialization. Separating those these three things into two parameters would give more flexibility. For instance

new View([options, parameters]) 

Everything in options could be attached to the view object itself, and everything in parameters is just used during initialization and then discarded. No more this.options, no more manually copying.

@tbranyen
Collaborator
tbranyen commented Apr 8, 2013

This is a necessary change. We have such ugly code in LayoutManager to getAllOptions.

https://github.com/tbranyen/backbone.layoutmanager/blob/master/backbone.layoutmanager.js#L395

This function smells so bad, we need a way to consolidate our options and I think everything on the instance makes perfect sense.

@braddunbar
Collaborator

Addressed in #2460.

@braddunbar
Collaborator

Out of curiosity, does anyone use default options (i.e. attached to the prototype)? I've always put defaults directly on the view instead.

@jstrimpel
@akre54
Collaborator
akre54 commented Apr 8, 2013

@braddunbar We use it for stuff like modal popups to determine whether to include action buttons (and their text), padding, etc. It's handy to have default options that inheriting classes or new instance methods can override or inherit.

@braddunbar
Collaborator

Thanks @akre54, @jstrimpel! Do you also use the values from the options object (i.e. this.options.foo)? If not, there probably isn't any added value right?

In other words, this code...

var View = Backbone.View.extend({
  options: {
    foo: 15,
    bar: false
  },
  initialize: function(options) {
    _.extend(this, _.pick(options, 'foo', 'bar'));
  }
});

...is functionally equivalent to...

var View = Backbone.View.extend({
  foo: 15,
  bar: false,
  initialize: function(options) {
    _.extend(this, _.pick(options, 'foo', 'bar'));
  }
});

...as long as you access foo as this.foo.

@akre54
Collaborator
akre54 commented Apr 8, 2013

I guess the main thing for me is the options hash looks a little cleaner and it's a little more obvious that they're meant to be overridden / passed in instead of being part of the instance, but you're probably right that it's functionally similar (and also quite easy to bring back).

Here's what I did for 0.9.9, when options first supported being called as a function:

class AbstractPopup extends BaseView
  templateData: -> _.extend @options, @model.toJSON()
  options: ->

class ActionPopup extends AbstractPopup
  options: -> _.extend super,
    fixed: true
    showActions: true
    text: 'Are you sure?'

  afterRender: ->
    @$el.addclass('fixed') if @options.fixed
    @$('.actions').hide() unless @options.showActions
    super

class LeaveGroupPopup extends ActionPopup
  options: -> _.extend super,
    text: 'Do you want to leave this group?'
    fixed: false
@tbranyen
Collaborator
tbranyen commented Apr 8, 2013

@akre54 except that it's not clean at all and ends up being very confusing when you assign something like template at the extend level and then try and override it at the invocation level. where is the most updated property? inside the instance or the options object?

@akre54
Collaborator
akre54 commented Apr 8, 2013

@tbranyen inside the this.options of the instance. This heirarchy has to support several different types of popups with different options behaviors, and this seemed like the most logical solution.

(new ActionPopup).options.text # 'Are you sure'
(new LeaveGroupPopup).options.text # 'Do you want to leave this group?'
(new LeaveGroupPopup({text: 'New text'})).options.text # 'New text'

Alternatively I could put those options on instance properties, but I feel like it would clutter up the instance properties with what are essentially optional parameters. Admittedly this is a contrived and ugly example.

@gsamokovarov

I never used the default prototype options property/function so I'm in favor of removing it given #2460 passes. Plus it creates this asymmetry between the View options and the Model/Collection options. Maybe its a matter of preference, but if class configuration properties are attached to the prototype, at least their inheritance will come for free.

@spadgos
spadgos commented Apr 8, 2013

I'm actually for keeping the options off the instance itself, for the reason @akre54 mentioned:

it's a little more obvious that they're meant to be overridden / passed in instead of being part of the instance

Values which live under options signify the parts of the view which can be configured by the code which instantiates it. The prototype can define defaults for these, but the values given to them are ultimately not its own responsibility. Copying them directly onto the instance removes this distinction of ownership.

@dgbeck
dgbeck commented Apr 8, 2013

Point raised by @akre54 and @spadgos makes sense. I think its the extra typing that turns me (and others) off. But out of curiosity do you ever come across "gray area" cases, where an option is determined both by the code which instantiates the view, and logic within the view itself. For example, where an option is passed in, and then perhaps manipulated or transformed, or may change during the life of the view?

@braddunbar braddunbar closed this in a22cbc7 Apr 9, 2013
@mateusmaso

Backbone.View always felt missing a solid event driven options with getter, setter and defaults values. Backbone.Model is also doing this with attributes so I wondered why not with views. I followed this thought and borrowed some ideas from Ember.js:

class DocumentSquareView extends Backbone.View

  defaults: 
    editable: true
    transient: false
    uploaded: false

  initialize: (options) ->
    @on("change:editable", @updateEditButton)

  ...

  updateEditButton: ->
    text = if @get('editable') then "Edit" else "Ok"
    @$('button').html(text)

  onChangeEdit: ->
    @set(editable: true)
@akre54
Collaborator
akre54 commented Sep 20, 2013

@mateusmaso check out Backbone.Attributes, a tiny plugin I just wrote to add getter and setter methods to any object. Would love any feedback for your usecase.

@henrylearn2rock

What would be the simplest and cleanest way of adding back the old behaviour for 1.1?

And would the code be added to a new section called Upgrading to 1.1 for backward compatibility?

Thanks!

@caseywebdev
Collaborator

You can do something like

app.View = Backbone.View.extend({
  constructor: function (options) {
    Backbone.View.apply(this, arguments);
    this.options = options;
  }
});

And then use app.View.extend({...}) for your view definitions.

@tgriesser
Collaborator

Or you could do:

var View = Backbone.View
Backbone.View = View.extend({
  constructor: function (options) {
    this.options = options;
    View.apply(this, arguments);
  }
});

to get it as it was previously (on all Backbone.View's)... and now this.options is available in initialize if you need it.

@henrylearn2rock

Thanks @caseywebdev and @tgriesser, but I thought options do not include: model, collection, el, id, className, tagName, attributes and events. I guess most code wouldn't care but to be backward compatible, maybe one should remove those keys from options?

@dgbeck
dgbeck commented Jan 5, 2014

Referencing this mini plugin as a way to attach white-listed initialization options directly to the view object that considers default values and required options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.