ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 396 forks source link

How to give views same functions (aka multiple inheritance)? #357

Closed binarykitchen closed 10 years ago

binarykitchen commented 10 years ago

Hello there

I know, Ractive's extend() does not support multiple inheritance but I am having an interesting case here. I would like to know how I can avoid code duplication here.

So, one view, the HomeView has this:

    var HomeView = AbstractView.extend({
        el: 'main',
        template: template(),

        init: function() {
            AbstractView.prototype.init.call(this);

            this.on('showOptionsMenu', this.showOptionsMenu);
            this.on('hideOptionsMenu', this.hideOptionsMenu);
        },

        showOptionsMenu: function() {
            this.set('optionsMenuVisible', true);
        },

        hideOptionsMenu: function() {
            this.set('optionsMenuVisible', false);
        },

       ... more stuff, own logic here (removed) ...
    });

and then the second view, the UserView has this:

    var UserVideoView = AbstractUserAccountView.extend({
        el: 'section.user',
        template: template(),

        init: function() {
            AbstractView.prototype.init.call(this);

            this.on('showOptionsMenu', this.showOptionsMenu);
            this.on('hideOptionsMenu', this.hideOptionsMenu);
        },

        showOptionsMenu: function() {
            this.set('optionsMenuVisible', true);
        },

        hideOptionsMenu: function() {
            this.set('optionsMenuVisible', false);
        },

       ... more stuff, own logic here (removed) ...
    });

As you can see, both extend from another view, have two same events (showOptionsMenu + hideOptionsMenu) and their own logic below.

What I do not like here is the duplicate code. Both views have the same application specific options menu.

How can I avoid code duplication here? Obviously I cannot create a new abstract class since both extend from different classes.

codler commented 10 years ago

You could use jQuery extend http://api.jquery.com/jquery.extend/

binarykitchen commented 10 years ago

No thanks, I do not use jQuery anymore ...

binarykitchen commented 10 years ago

Is there a chance to improve Ractive's extend() function to allow multiple parents?

Rich-Harris commented 10 years ago

I suppose it's a possibility. I quite like single inheritance though because it's basically just bog-standard JavaScript prototypal inheritance, which most JS devs are extremely comfortable with, and because of that it behaves predictably:

HomeView instanceof AbstractView; // true
HomeView instanceof Ractive; // true
UserVideoView instanceof HomeView; //false

AbstractView.prototype.foo = function () { alert( 'bar' ); };
homeView.foo(); // alerts 'bar'

I think there's a lot to be said for going with the grain of a language in this way. With multiple inheritance you have to start overcoming all sorts of problems - how do we deal with the fact that there's no longer a clear prototype chain? What happens if two or more 'parents' have different methods of the same name? In Ractive's case, what happens if SomeView and OtherView both have a partials or transitions registry - should they be combined? And how do we implement all this with a syntax that feels like idiomatic JavaScript?

That said there are absolutely situations where you want to share code between classes but not all descendants of their mutual ancestor.

One way would be to simply do this:

var showOptionsMenu = function () {
  this.set('optionsMenuVisible', true);
};

var hideOptionsMenu = function () {
  this.set('optionsMenuVisible', false);
};

var bindEventHandlers = function (instance) {
  instance.on({
    // note that we could call the functions directly here, they
    // will still have the correct context. But for the present illustration
    // we'll assume we want to have them as methods of the instance
    showOptionsMenu: instance.showOptionsMenu,
    hideOptionsMenu: instance.hideOptionsMenu
  });
};

var HomeView = AbstractView.extend({
  init: function () {
    this._super(); // easier way to call AbstractView.prototype.init
    bindEventHandlers(this);
  },
  showOptionsMenu: showOptionsMenu,
  hideOptionsMenu: hideOptionsMenu
  ...
});

var UserVideoView = AbstractUserAccountView.extend({
  init: function () {
    AbstractView.prototype.init.call(this);
    // or this._super() if there's no class with an `init` method in the
    // prototype chain between UserVideoView and AbstractView

    bindEventHandlers(this);
  },
  showOptionsMenu: showOptionsMenu,
  hideOptionsMenu: hideOptionsMenu
  ...
});

...though having lots of itty-bitty methods probably isn't ideal, especially with AMD or other module systems.

So an alternative would be to define a mixin helper (which, actually, is basically the same as using $.extend()):

var mixin = function(object, mixin) {
  for (var prop in mixin) {
    if (mixin.hasOwnProperty(prop)) {
      object[prop] = mixin[prop];
    }
  }
};

var withOptionsMenu = {
  init: function () {
    AbstractView.prototype.init.call(this);
    this.on({
      showOptionsMenu: this.showOptionsMenu,
      hideOptionsMenu: this.hideOptionsMenu
    });
  },
  showOptionsMenu: function () {
    this.set('optionsMenuVisible', true);
  },
  hideOptionsMenu: function () {
    this.set('optionsMenuVisible', false);
  }
};

mixin(HomeView, withOptionsMenu);
mixin(UserVideoView, withOptionsMenu);

Would that work?

Internally, Ractive uses a mixin-like strategy quite extensively - e.g. there are a whole load of classes that behave as though they inherit from a Mustache class, even though in reality there's no such thing (just a load of shared methods, and an initMustache() function that each of those classes call on themselves).

If you haven't read Angus Croll's piece on mixins, it's well worth it (though I don't tend to use functional mixins myself): http://javascriptweblog.wordpress.com/2011/05/31/a-fresh-look-at-javascript-mixins/

As an aside, I've been wondering whether methods that correspond to event names should be called when those events are fired. I.e. instead of having to do this.on('showOptionsMenu', this.showOptionsMenu), the method is called automatically when the showOptionsMenu event is fired. Might be slightly surprising though, and would be a breaking change for some people. Thoughts?

codler commented 10 years ago

As an aside, I've been wondering whether methods that correspond to event names should be called when those events are fired. I.e. instead of having to do this.on('showOptionsMenu', this.showOptionsMenu), the method is called automatically when the showOptionsMenu event is fired. Might be slightly surprising though, and would be a breaking change for some people. Thoughts?

It is an cool idea! but I think it would lead to be too magical maybe? and might conflict with existing context properties such as init, data etc

Rich-Harris commented 10 years ago

@codler you're always the voice of reason! This is at least the third time you've dissuaded me from implementing 'convenience' features that would have caused confusion and regret.

How about this? Provides the same level of convenience but without the surprise:

var HomeView = AbstractView.extend({
  el: 'main',
  template: template(),

  on: {
    showOptionsMenu: function() {
      this.set('optionsMenuVisible', true);
    },

    hideOptionsMenu: function() {
      this.set('optionsMenuVisible', false);
    },
  },

  ...
});

There's a dual advantage here - as well as saving code, it means that we're not overwriting the init() method - so we don't need to do this._super() or AbstractView.prototype.init.call(this). I'm imagining that the properties of on would accumulate with each subclass, so that...

BasementView = HomeView.extend({
  on: {
    toggleOptionsMenu: function () {
      this.toggle('optionsMenuVisible');
    }
  }
});

...would response to showOptionsMenu and hideOptionsMenu events as well as toggleOptionsMenu events.

Actually I've just opened a separate issue (#360) for this so that I don't derail this thread any further.

binarykitchen commented 10 years ago

@Rich-Harris I agree that multiple inheritance can be dangerous and confusing. But look over at the C++ world, there you can do it.

Since C++ is more complicated, it is more suitable for experienced hackers. What I am trying to say is that if you fuck up, then it is your mess. I wouldn't recommend multiple inheritance to intermediate JS hackers either but I do hack for over 20 years and know what I am doing here ;) In my case it pays to avoid code duplication in the application logic for faster prototyping.

I've been thinking a lot about all that and done some research. But haven't found a good solution yet. Your two suggestions with the bindEventHandlers and mixin would work but both are not able to call multiple parent constructors (when the child instance has multiple parents) ...

Rich-Harris commented 10 years ago

Ah, well I'm afraid I'm a native JavaScripter! So for me, things like MI are interesting CS topics that I enjoy reading about, but mostly because I'm glad I don't have to worry about them...

In the case of Ractive.extend(), there are no parent constructors to speak of, only the different init methods - so if the mixin approach works for adding methods to HomeView.prototype and UserVideoView.prototype (for example), then maybe the way to achieve MI viz. calling multiple parent constructors is as simple as

HomeView.prototype.init = function () {
  AbstractView.prototype.init.call(this);
  ViewWithOptionsMenu.prototype.init.call(this);
};

UserVideoView.prototype.init = function () {
  AbstractView.prototype.init.call(this);
  ViewWithOptionsMenu.prototype.init.call(this);
};

Admittedly it makes you want to gouge your eyes out, but it could be refactored:

inheritFromParents = function (instance) {
  var parent, parents = Array.prototype.slice.call(arguments, 1);
  while ( parent = parents.shift() ) {
    parent.prototype.init && parent.prototype.init.call(instance);
  }
};

HomeView.prototype.init = function () {
  inheritFromParents(this, AbstractView, ViewWithOptionsMenu);
};

// ditto for UserVideoView

What do you reckon?

codler commented 10 years ago

Will this work?

AbstractView.extend(ViewWithOptionsMenu).extend({
  init: function() {
    this._super();
  }
})
martypdx commented 10 years ago

As the saying goes in OOP, "favor composition over inheritance". I would encapsulate the common menu functionality in a (Ractive) Menu component used in the main template as menu, not MI the main component. Just chiming in because I think getting components to work well solves (or flat out avoids) a whole slew of issues.

binarykitchen commented 10 years ago

@martypdx Good point. I've been thinking about this. This might reduce some code duplication but is not the perfect solution. I already tried composition but there was still duplicate code because you have to create and reference to the common menu component from both view instances. And if you make changes at one place, you still have to remember to modify at the other place. That's a no go for me, sorry.

@codler Nope, I haven't tried that yet. Will calling .extend().extend() twice work? @Rich-Harris can you comment on this?

@Rich-Harris inheritFromParents() is a nice mixin. I will try this ... but still, multiple inheritance would be great! Currently I am eyeing on https://github.com/nicolas-van/ring.js but am hesitant to use it because it will bloat up my whole JS code ...

binarykitchen commented 10 years ago

@martypdx PS: also another disadvantage of composition is to maintain correct references to the target instance.

martypdx commented 10 years ago

@binarykitchen Without seeing your template, it's hard to say. But doesn't using a component both eliminate the duplication and give you implicit reference?

h1 {{title}}
.someContainer
    menu(somedata="{{meaningful_data_or_context}}")
.otherStuff

And then the .extend component encapsulates the events

MenuOptions = Ractive.extend({
  template: menuOptionstemplate,
  init: function() {
    this.on('showOptionsMenu', this.showOptionsMenu);
    this.on('hideOptionsMenu', this.hideOptionsMenu);
  },
  on: {
    showOptionsMenu: function() {
      this.set('optionsMenuVisible', true);
    },
    hideOptionsMenu: function() {
      this.set('optionsMenuVisible', false);
    }
  ...
}
binarykitchen commented 10 years ago

Thanks @martypdx but I am sorry, I do not see the composition pattern here and still doubt that it can eliminate 100% of the code duplication.

Rich-Harris commented 10 years ago

@binarykitchen Yes, @codler's idea works perfectly. In fact, here's the ring.js demo recreated with Ractive.extend():

var Human = Ractive.extend({
  talk: function() {
    return "hello";
  }
});

var Spider = Ractive.extend({
  climb: function() {
    return "climbing";
  }
});

var SpiderMan = Human.extend( Spider.prototype ).extend({
    talk: function() {
        return this.$super() + ", my name is Peter Parker";
    }
});

var spiderman = new SpiderMan();
console.log(spiderman.talk());

In fact the more I think about it, the less benefit I see to any kind of multiple inheritance implementation - I just don't see anything that MI brings to the table here that isn't already possible with mixins, or chained .extend() calls, or (depending on the app) components.

I don't want to rag on someone else's library, but since you mention ring.js as a possible solution, I should point out that it breaks one of the central features of prototypal inheritance, and neatly illustrates why MI goes against the grain of the language:

// after the code on http://ringjs.neoname.eu/
Human.prototype.foo = function () { alert( 'the prototype chain is intact!' ); };

try {
  spiderman.foo();
} catch ( err ) {
  alert( 'The prototype chain is not intact. We broke JavaScript :(' );
}

Guess which alert we get! Maybe you think 'doesn't matter, I don't modify prototypes after they've been created' - but some people do (e.g. we might want to have different versions of a method depending on locale, or browser capabilities), and JavaScript lets us do that. We break it at our peril.

binarykitchen commented 10 years ago

@Rich-Harris Thanks! Just checking: Did you really try the cool Spiderman example with ring.js? And can we already call .extend() multiple times for Ractive views (chained extends)?

So, back to your code snippet saying MI goes against the grain of the language. You are right if you read the ECMA5 standards under chapter "Objects" at http://www.ecma-international.org/ecma-262/5.1/#sec-4.2.1 which says ECMAScript does not use classes such as those in C++, Smalltalk, or Java...

But I think JS has been stuck for too long time during the old DHTML days. Javascript will evolve with the upcoming ES standards I think. JS has to compete with other languages. Look over at C++, there this kind of polymorphism is very popular and often used.

Still, I would welcome support for chained extend() calls to reduce code duplication!

codler commented 10 years ago

@binarykitchen yes you can call .extend() multiple times and its chainable.

binarykitchen commented 10 years ago

@codler I tried to chain .extend() multiple times but it is not working. Custom methods are not inherited.

Rich-Harris commented 10 years ago

@binarykitchen it definitely works! Make sure you're doing e.g. .extend(SomeClass.prototype) and not extend(SomeClass). Run this code in the console if you don't believe me:

withFoo = {
  foo: function () {
    alert( 'foo called' );
  }
};

withBar = {
  bar: function () {
    alert( 'bar called' );
  }
};

WithFooBar = Ractive.extend( withFoo ).extend( withBar );

wfb = new WithFooBar();
wfb.foo();
wfb.bar();

Yes, I tried the ring.js example.

Sure, JS is evolving (though I'd argue the language's evolution is less dramatic than the culture that surrounds it), and it should be possible to achieve MI with ES6 proxies one day if that's your bag. But we can't change the rules and make it make sense with ES3/5 any more than we can polyfill new CSS features. Since there are so many decent (and idiomatic) alternatives I think we should focus our energies elsewhere.

And on that note I'm going to close this issue, so I can get to work on some of the others ;)

binarykitchen commented 10 years ago

@RactiveJS Ah, with .prototype it works now. Okay, can we add this in the documentation? It is not written there that it's chainable.

Right, in that case not all parent constructors are called. Hence I think your suggestion to add

inheritFromParents = function (instance) {
  var parent, parents = Array.prototype.slice.call(arguments, 1);
  while ( parent = parents.shift() ) {
    parent.prototype.init && parent.prototype.init.call(instance);
  }
};

would be great! Can this be added without introducing breaking changes? Just asking.

PS: Alright, I agree re: JS evolution. No further comments :)