Drop this.options support / remove Marionette.getOption

[samccone]

6485c5b#diff-73a2d8c7d9cf5ae93d1ef423f27243d2R15

#738 (comment)

[jamesplease]

I feel like this has been mentioned somewhere else, but should we also get rid of this.getOption once this is implemented?

[samccone]

yup good call

[jamesplease]

@samccone @jasonLaster @cobbweb this is going to be a third huge refactor. So much relies on getOption it's a bit worrisome.

Once getOption is gone, we're really going to need to answer the question regarding which properties we want to be class-level-only properties (things that are only considered when added to the prototype), and which things are special, and are 'latched' onto and saved to the instance when you instantiate it.

To give an example, Backbone selectively pulls out the following options and attaches it to a View:

model, collection, el, id, className, tagName, attributes and events.

On the other hand, template is a class-level-only property that you can't pass in on initialization. I admit that I don't have the reasoning behind this decision sorted out myself. But as a team we will certainly need to have it sorted out to remove getOption!

[jamesplease]

I mentioned in the above post that we need to select the options we want to be Class-level and those that we will attach directly to the instance. I think it makes sense to go about this in the same way that Backbone does, which is to specify an array of properties to merge, and then merge them within the constructor. To use the same example as above, you can see that Backbone doesn't merge all of the possible options of a view, because it doesn't let you specify a template during initialization.

If we're fine with that implementation, then the next question is to write up the properties we want to make class- and instance- level for each Class. Maybe we want everything to be instance-level? I'm not sure. We should give more thought into why Backbone makes the distinction.

The document here is a place for us to specify the class and instance level options. @thejameskyle @jasonLaster @ahumphreys87 I need your emails to give you write permissions...just post them to me in gitter sometime.

Another question is how to handle options that don't exist. I propose we just go about it in the same way that Backbone does and not try to add any custom handling. We cna just let the code run its course and if the user tries to do something that requires an option they haven't set then shit will e'splode.

[jamesplease]

Okay, so a first pass at the document is complete. I think I covered every class. Everything is instance-level except a few things, so I'd like to explain my decision there.

The rule I took was: Backbone attaches everything as an instance-level property except template. Consequently, anything related to templating was attached to the Class. What this gives us are templateHelpers for views and itemViewContainer for compositeView.

The big question is: why did Backbone not specify template as a viewOption? Is the template too fundamental to the Class to be pulled out separately? That seems likely to me.

Well, anyway, if we went with this 99.9% of apps would not break. Only folks who pass in itemViewContainer or templateHelpers, which I would wager are more likely to be attached to the Class anyway (alongside template). Either way it would be a part of v2 so breaking changes are fine – but this won't be as bad as I initially thought.

[samccone]

@jmeas see my comments in the gitter room for my current thoughts

[fenduru]

Out of curiosity, what do you see as being the best practice regarding instance-level-options when extending Marionette.View? Will I be able to supply a suplemental list of properties I want copied over?

[jamesplease]

@fenduru just a note: these changes are still in the discussion state. We're not even sure if it'll land in v2.

With that said, one possibility we had was a property that might be called something like mergeOptions. It would be an array of values to automatically attach to this on init. This pattern is taken directly from Backbone.

Then in the constructor of each base class it pulls in those values from the passed-in options.

Here's an example of how it might work:

var MyViewClass = Marionette.ItemView.extend({
  mergeOptions: [ 'color', 'type' ]
});
var newView = new MyViewClass({
  color: '#fff',
  type: 'bold',
  age: 26
});

// Equal to '#fff'
newView.color;
// equal to 'bold'
newView.type;
// undefined
newView.age;

Not to repeat myself but these are just thoughts still being brainstormed. Nothing is finalized yet :)

[fenduru]

Looks good, I would be very happy with that.

[jasonLaster]

@samccone what is the status here?

[jamesplease]

This is too big a change for v2, and doesn't make much sense in the context of our other changes. We will hold this off for a future major release. Removing the v2 label.

[jamesplease]

While I think we should keep this.option for a little bit longer, I do think we should introduce the future mergeOptions (or whatever we want to call it) in 2.x, so that people can start migrating over.

[samccone]

The more I think about this, this.options does not make much sense in backbone core, however in marionette it makes a ton of sense. For that reason I do not think we should ever drop it.

[jamesplease]

I agree that something like it makes sense, but I think we can do even better than this.options! I'll make up a PR sometime and we can discuss then :)

[jamesplease]

That PR is #1583

[cmaher]

In relation to mergeOptions and the prototype chain (as discussed in #1583)

Marionette.extend = function (protoProps, staticProps) {
  child = Backbone.Model.extend.call(this, protoProps, staticProps);
  child.prototype.mergeOptions = _.union(this.prototype.mergeOptions, child.prototype.mergeOptions);
  return child;
};

Something like mergeOptions would save me a lot of ugly code and would help document classes by explicitly listing what options they accept.

[jamiebuilds]

@cmaher I don't think we should go down the road of overriding the extend method with one-off stuff like this.

[jamesplease]

I think we should talk about that possibility more...first I'm going to open a PR/issue on Backbone about it to see what they think.

[samccone]

I will go ahead and predict the future 💀

[jamesplease]

rofl. Worth a try tho'

[cmaher]

Overriding extend opens us up to the possibility of doing a whole bunch of stuff in a nice way. But if we don't want to do that, we can do something like:

// in a constructor somewhere
var T = this;
while (T.__super__) {
  this.prototype.mergeOptions = _.union(this.prototype.mergeOptions, T.prototype.mergeOptions)
  T = T.__super__.constructor
}

which I am not super-jazzed about.

[jamesplease]

Given the outcome of jashkenas/backbone#3290 I'm in favor of Marionette.extend, Marionette.Model and Marionette.Collection. We can add Backbone's unit tests for the model and collection to run our Classes against to ensure that they remain backwards compatible.

[jamesplease]

Moved to #1796