mixin pattern?

[pixelzoom]

I'm creating this issue in scenery, since it appears to be the sole place where the "mixin" pattern is being used. This discussion started in Slack developer channel on 10/21/2017, and I transferred the discussion to this issue.

#696 (Accessibility mixin uses features of Node) prompted me to ask:

Question about mixins in general… They should not use features of the thing(s) that they will be mixed into, correct?

[pixelzoom]

@samreid said:

maybe that would be OK if it was clearly documented

[pixelzoom]

Accessibility is using numerous properties on Node and its supertype. That seems very wrong. Similar to a supertype using features of a subtype.

[pixelzoom]

@zepumph said:

I think that the priorities of the accessibility mixin were to keep 2000 more lines out of Node while creating a usable API for phet devs to implement a11y into sims. I don't think it was ever designed to be independent of Node's implementation. @jessegreenberg would definitely be able to speak to it more.

[pixelzoom]

AccessiblePeer has a similar problem. Not only does it mix itself into Poolable, but it requires Poolable. This is not mixin, this is subtyping.

[pixelzoom]

@zepumph said:

If "AccessibleNode.js" was created with a11y features rather than using a mixin, then wouldn't each usage of new Node() need be converted to use the subtype?

[pixelzoom]

I’m not questioning the fact that this pulled code out of Node. I’m questioning how the mixin pattern is being applied. Happy to have someone point me to a version of mixin that matches what is being done here.

https://en.wikipedia.org/wiki/Mixin “Mixin programming is a style of software development, in which units of functionality are created in a class and then mixed in with other classes. A mixin class acts as the parent class, containing the desired functionality. A subclass can then inherit or simply reuse this functionality, but not as a means of specialization. Typically, the mixin will export the desired functionality to a child class, without creating a rigid, single “is a” relationship. Here lies the important difference between the concepts of mixins and inheritance, in that the child class can still inherit all the features of the parent class, but, the semantics about the child “being a kind of” the parent need not be necessarily applied.”

I have yet to find a definition of mixin where it functions as a subtype - that is, can use features of the thing that it’s being mixed into.

[pixelzoom]

@jonathanolson said:

I understand it is an atypical usage of the mixin pattern, but what would you recommend instead Include those in Node itself?

[pixelzoom]

Atypical usage is putting it nicely. Correct me if I’m wrong. But it looks like we’re just pulling some functionality out into a file2, so that file1 doesn’t get too big. While still allowing file2 to reach over into file1 and use whatever it wants. And it looks like this pattern is not exclusive to Accessibility. I see that other mixins (Paintable, RectangleStatefulDrawable,…) access properties of the subtype that they will be mixed into.

This is not mixin. What’s currently being done is closer to the Decorator pattern — though that pattern typically applies to instances, not types.

[pixelzoom]

@jonathanolson said:

The JS example at https://en.m.wikipedia.org/wiki/Mixin has a mixin (NameMixin) that assumes the existence of fields that the type it is mixed into has.
But if another name is more appropriate, it would be good to use it.

[pixelzoom]

@jessegreenberg said:

Would traits better describe what we are doing?
https://en.wikipedia.org/wiki/Trait_(computer_programming)

I am not sure how accurate this resource is, but from https://www.barbarianmeetscoding.com/blog/2016/01/04/safer-javascript-object-composition-with-traits-and-traits-dot-js/

Traits attempt to solve these problems by providing a way to reuse code and behavior just like we do with mixins but in a safer fashion that will let us:
handle conflicts between traits and be warned when conflicts occur
define requirements in our traits that must be satisfied before certain features can be used

[pixelzoom]

@samreid said:

In @jonathanolson example from yesterday, if you had a PlaysBasketball or KnowsKungFu mixin, it makes sense that you could only mix that in to certain types. Like Human with KungFu or Cat with KungFu but maybe not Boolean with KungFu

Subclasses can call methods on their parent classes, so mixins should be able to call methods on their mixees, right?

[pixelzoom]

@jessegreenberg said:

I guess that makes sense to me. @pixelzoom thought it felt the other way around, like the parent class calling a method on the subclass

[pixelzoom]

@samreid said:

Good point @jessegreenberg

[pixelzoom]

@jonathanolson said:

I do think more in terms of traits (particularly from Scala), so trait would match what we are doing

[pixelzoom]

Traits would be closer, but would still require some changes. From one of the seminal papers on traits (http://scg.unibe.ch/archive/papers/Scha03aTraits.pdf, p2):

• A trait provides a set of methods that implement behavior.
• A trait requires a set of methods that serve as parameters for the provided behavior.
• Traits do not implement any state variables, and the methods provided by traits never access state variables directly.

In the case of Accessibility, this means that it can require the presence of Node methods, but cannot access Node state variables. E.g. Accessibility cannot access _children, but can call method getChildren.

We’d probably punt on enforcing the presence of the required methods - or perhaps provide a list of types that a Trait is allowed to be composed with. We’d probably also disallow naming conflicts, rather than try to implement a way to explicitly resolve conflicts.

So my current inclination (which I’m still ruminating on) is to:
• call these things Traits
• replace “extend” with “compose” (a new function)
• in a Trait’s definition, specify the types (and Traits?) that it can be composed with
• in “compose”, verify that we’re composing with a valid type, and fail if there are naming conflicts

[pixelzoom]

Btw… “Programming in Scala” (p 230, “To trait, or not to trait?“) recommends:

If the behavior will not be reused, then make it a concrete class. It is not reusable behavior after all. If it might be reused in multiple, unrelated classes, make it a trait.

I am not aware of any current PhET “mixin” that is being used (or is intended to be usable) in more than 1 place - so not in the typical situation where a mixin or trait would be used.

[pixelzoom]

(Slack discussion included all of the above, non-Slack discussion continues below.)

[samreid]

@pixelzoom it wasn't clear whether you had acknowledged @jonathanolson's comment #700 (comment)

He is referring to https://en.wikipedia.org/wiki/Mixin#In_JavaScript which has:

// This example may be contrived.
// It's an attempt to clean up the previous, broken example.
var Halfling = function (fName, lName) {
    this.firstName = fName;
    this.lastName = lName;
}

var NameMixin = {
    fullName: function () {
        return this.firstName + ' ' + this.lastName;
    },
    rename: function(first, last) {
        this.firstName = first;
        this.lastName = last;
        return this;
    }
};

var sam = new Halfling('Sam', 'Lowry');
var frodo = new Halfling('Freeda', 'Baggs');

// Mixin the other methods
_.extend(Halfling.prototype, NameMixin);

// Now the Halfling objects have access to the NameMixin methods
sam.rename('Samwise', 'Gamgee');
frodo.rename('Frodo', 'Baggins');

As @jonathanolson said, "[it] assumes the existence of fields that the type it is mixed into has" in the same way that our Accessibility mixin assumes the existence of Node methods and properties.

[pixelzoom]

@pixelzoom it wasn't clear whether you had acknowledged @jonathanolson's comment ....

I believe that example is incorrect.

[pixelzoom]

I also didn't reply to this... @samreid said:

Subclasses can call methods on their parent classes, so mixins should be able to call methods on their mixees, right?

Mixins function as base classes (superclasses) not subclasses.

[samreid]

True, but sometimes parent classes have abstract methods which must be implemented in the child class.

[pixelzoom]

I fail to see any relation to what is being done here. PhET's "mixins" are using things that are both declared and defined in the subclass.

[samreid]

An abstract class explicitly defines methods which must be implemented in the subclass. A mixin implicitly defines methods which must be implemented in the mixee.

[pixelzoom]

A mixin implicitly defines methods which must be implemented in the mix.

Where are you getting this information? And how do you rationalize multiple mixins all "implicitly defining" Node children, parent, etc.

[pixelzoom]

A trait (not a mixin) declares methods that must be implemented by things (classes and other traits) that the trait may be composed with.

[jonathanolson]

AccessiblePeer has a similar problem. Not only does it mix itself into Poolable, but it requires Poolable. This is not mixin, this is subtyping.

It mixes Poolable into itself. And actual subtyping does not seem like an option here, since Poolable truly is used (a) on types with very different supertypes, and (b) does not have any type-specific information, i.e. it can be mixed into basically anything.

I am not aware of any current PhET "mixin" that is being used (or is intended to be usable) in more than 1 place - so not in the typical situation where a mixin or trait would be used.

• Poolable is mixed in 24 times (and many times through SelfDrawable.Poolable)
• PaintableStatelessDrawable is mixed in 7 times
• PaintableStatefulDrawable is mixed in 5 times
• CircleStatefulDrawable is mixed in 2 times
• ImageStatefulDrawable is mixed in 3 times
• LineStatefulDrawable is mixed in 1 time (will be used more in the future with WebGL)
• PathStatefulDrawable is mixed in 1 time (will be used more in the future with WebGL)
• RectangleStatefulDrawable is mixed in 3 times
• TextStatefulDrawable is mixed in 2 times
• Paintable is mixed in 2 times
• Accessibility is mixed in 1 time
• Leaf is mixed in 1 time (potentially more general-use)

For an example:

• RectangleWebGLDrawable's inheritance is WebGLSelfDrawable:SelfDrawable:Drawable
• CircleSVGDrawable's inheritance is SVGSelfDrawable:SelfDrawable:Drawable
• TextCanvasDrawable's inheritance is CanvasSelfDrawable:SelfDrawable:Drawable
• ImageWebGLDrawable's inheritance is WebGLSelfDrawable:SelfDrawable:Drawable

Of those, everything except ImageWebGLDrawable have some concept of being "paintable" (fill/stroke). TextCanvasDrawable doesn't need to save the state of the current paint, so it uses PaintableStatelessDrawable, but the other two do, using PaintableStatefulDrawable. This isn't something that can be solved with subtyping, as it's a clear "multiple inheritance" issue, where we need to mix-and-match render type (Canvas/SVG/WebGL/DOM), node type (Circle/Line/Rectangle/Text/Image/etc.), and at a higher level paintability and style (PaintableStatelessDrawable/PaintableStatefulDrawable).

Note that these all expect things to be SelfDrawables, and there is a lot of typing constraints. In addition, it could use some cleanup (some of it is somewhat unintuitive), but there's clearly a need for handling "multiple inheritance" style issues here.

Notably, of the other cases:

• Accessibility is only used in one place (and presumably only will ever be), and would be a candidate to include in Node directly.
• Paintable is never mixed into objects with different supertypes, and thus could (currently) be reformulated as a supertype (e.g. Path inherits PaintableNode inherits Node, same for Text).
• Poolable is the only one that doesn't assume it's mixed into something of a specific type.

This is not mixin. What’s currently being done is closer to the Decorator pattern — though that pattern typically applies to instances, not types.

So my current inclination (which I’m still ruminating on) is to:
• call these things Traits
• replace “extend” with “compose” (a new function)
• in a Trait’s definition, specify the types (and Traits?) that it can be composed with
• in “compose”, verify that we’re composing with a valid type, and fail if there are naming conflicts

I'm fine with whatever name we choose. The pattern StaticMixinReference.mixin( Type ) was chosen previously in phetsims/phet-core#16. Are you recommending replacing this with StaticTraitReference.compose( Type )?

Assertion checks to ensure it's a proper expected type sound good to me (might need to use PHET_CORE/inheritance to grab the full prototype chain for inspection, as instanceof won't work for type references).

Additionally, I'd like to note that JavaScript itself (with prototype/type modification) allows a lot of flexibility to implement behavior that can't be done in some other languages (like Poolable placing the pool object on the type itself, which can significantly ease debugging). Type modification (like mixins/traits) has so far been most helpful sharing behavior between types that all have a shared supertype, so I don't want to exclude that in the future.

[pixelzoom]

I stand corrected - there are indeed many PhET "mixins" that are reused extensively. So you can ignore #700 (comment).

With the exception of Poolable, I still think it's problematic that the other "mixins" require specific functionality in the type that they are mixed into. Smells like trait, not mixin. So I stand by #700 (comment).

@jonathanolson said:

Additionally, I'd like to note that JavaScript itself (with prototype/type modification) allows a lot of flexibility to implement behavior that can't be done in some other languages (like Poolable placing the pool object on the type itself, which can significantly ease debugging). Type modification (like mixins/traits) has so far been most helpful sharing behavior between types that all have a shared supertype, so I don't want to exclude that in the future.

I'm not suggesting that we shouldn't think outside the box. But JavaScript also gives you the "flexibility" to break encapsulation, access things you shouldn't, violate proper application of patterns, and generally create maintenance problems. So I think we should exercise restraint when extending/adapting existing design patterns - it's confusing to developers who are familiar with those patterns, and does a disservice to new developers who are still expanding their knowledge of design patterns.

[pixelzoom]

Task for 10/26/17 developer meeting is to decide whether to (a) leave as is, or (b) rename things that are not following the Mixin pattern. If (b), decide on a name.

[pixelzoom]

10/26/2017 dev meeting notes:
Let's go with Trait. We'll consider Mixin as a Trait with no requirements on the thing its mixed into.
Change mix: to mixInto:.
Add assertion to do type checks for what type(s) a Trait can be mixed into.
@jonathanolson will handle the change.

[jonathanolson]

Implemented renamings and type-check assertions. Aqua tests passing.