Page MenuHomePhabricator

Restrict the class to which a mixin may be applied
Closed, DeclinedPublic

Description

Currently, mixins sometimes make an undocumented (and unchecked) assumption that any instance will be an instance of some other class. For example, in VisualEditor, any class that mixes in ve.ce.ActiveNode must inherit from ve.ce.Node. Something like the following behaviour would be good:

OO.initMixinClass( ve.ce.ActiveNode, ve.ce.Node );
MyClass = function () {};
// Throws Error: ve.ce.ActiveNode can only be applied to a subclass of ve.ce.Node
OO.mixinClass( MyClass, ve.ce.ActiveNode );

See also: T92540 ("Require classes being mixed in to be plain objects")

Event Timeline

This is essentially the same problem as one could run into with PHP Traits: http://php.net/manual/en/language.oop5.traits.php. A Trait in PHP is quite similar in behaviour to what we do with mixins.

There are numerous posts, questions and articles about best practices around traits. Among them, a common anti-pattern is Traits making assumptions about what the effective parent class will be.

https://stackoverflow.com/questions/41712413/may-i-use-properties-from-a-parent-class-in-a-trait
https://eliasvo.wordpress.com/2015/06/07/php-traits-if-they-werent-evil-theyd-be-funny/

Rather than trying to enforce or detect it, I'd prefer we solve the problem by not making these assumptions in the first place.

Two common solutions:

  1. Injection

Instead of using a mixin, one could instead make the mixin a standalone class. This could then be instantiated in the constructor of the main class, or injected as constructor parameter. This also allows the (former mixin) class to be configured if needed. Any methods that need to exist in the main class must then be created manually, but the implementation can be a simple stub that calls out to the other instance.

Benefits:

  • Shared code.
  • Ability to share a single instance by injection, or to construct new ones where needed.
  • Composes by injection (tree-like) instead of composing by mixing (flattened).
  • No method or property name conflicts.
  • More explicit.
  • Easier to test.
function FormerMixin() { .. }

function Main( speed, fm ) {
  this.speed = speed;
  this.fm = fm || new FormerMixin( this.speed );
}
Main#walk = function ( steps ) {
  return fm.walk( steps );
};
  1. Pure sharing

If a mixin isn't worth turning into an instantiable class on its own, a mixin can still be a good way to share and re-use logic that is needed in multiple places. In that case the only requirement to avoid knowledge of parent classes, is to require any information to be passed to the methods by parameter.

function Mixin() { .. }
Mixin#walkHelper = ...;

function Main( speed ) {
  this.speed = speed;
}
OO.mixinClass( Main, Mixin );

Main#walk = function ( steps ) {
  return this.walkHelper( this.speed, steps );
};

Solution 2 is probably what you'd want here. The requirement for having a ve.Node would be solved by passing it as parameter to the mixin method (if needed), instead of the mixin method making assumptions about this. The required would be verified via the @param signature and/or a run-time conditional.

Krinkle triaged this task as Medium priority.Jun 14 2017, 6:24 PM
Krinkle moved this task from Inbox to Backlog on the OOjs core board.