Improved pattern for mixin and trait

[jessegreenberg]

From developer meeting discussion (phetsims/phet-core#54) @samreid found a potentially better way for us to implement mixin/ trait pattern. Here is the resource

https://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/

@jonathanolson said he would be interested in evaluating.

[jonathanolson]

This looks quite promising, and "feels" cleaner, with the exception of constructors (we'd need to use rest/spread operators for constructors instead of initializers, which is fine) and the fact that we'd presumably need to either (a) turn almost everything into ES5 classes, for the Node mixins, or (b) figure out something that works with both.

Follow-up article is needed: https://justinfagnani.com/2016/01/07/enhancing-mixins-with-decorator-functions/, and the helper library is at https://github.com/justinfagnani/mixwith.js.

[samreid]

I agree this looks like the appropriate way to proceed. The class-factory pattern is

const MyMixin = Superclass => class extends Superclass {
  // mixin methods go here
};

class A extends MyMixin(MyBaseClass) {}

Our existing mixins are:

• Poolable
• AccessibleSlider
• PaintableStatelessDrawable
• PaintableStatefulDrawable
• CircleStatefulDrawable
• ImageStatefulDrawable
• PathStatefulDrawable
• RectangleStatefulDrawable
• TextStatefulDrawable
• Leaf
• Paintable
• EnabledNode
• EnabledPhetioObject
• AccessibleNumberSpinner
• AccessibleValueHandler

So it seems to move toward the class-factory pattern, we would need to change every type that these are mixed into to be class definitions. Should we chip away at this?

Also, we currently have AccessibleSlider "extends" AccessibleValueHandler--how would that be done with the class-factory pattern?

Tagging for discussion at developer meeting.

[jonathanolson]

Also, we can create inherit-based versions of this for use with our current inherit-based types.

[mattpen]

Dev Meeting: This seems promising, but @ariel-phet said it should be on hold until density is published.

[jonathanolson]

Somewhat ironically, I almost needed to use this style of mixin to properly handle a case in Density (so it came first). Tagging to view/discuss during dev meeting.

[samreid]

Today @jonathanolson described how he is using this pattern in Density & Buoyancy in DensityAndBuoyancyModel.js, and reported that it is working well. He said it is a likely candidate for replacing old mixin patterns, but may take more effort to use with inherit instead of class.

@jbphet said this may be an opportunity for us to upgrade some types to class instead of inherit. Perhaps Vector2.

The current proposal is to use this pattern to create new mixins. Then when we have more experience we can decide whether to backport this pattern to existing mixin patterns. @jonathanolson invited developers to reach out to him for collaboration when starting new mixins.

We also agreed to unmark for developer meeting and add a calendar entry to re-evaluate in 6 months or so. @ariel-phet can you please add a calendar entry?

[ariel-phet]

Calendar reminder made

[pixelzoom]

In phetsims/sun#585, I've been tasked with converting components to use EnabledComponent. It (and its sub-mixins) are apparently "old style". Do we really want to convert a bunch of things to use an old pattern? Changing the status of this issue and labeling for developer meeting with high priority.

[samreid]

The improved pattern for mixin requires class. Ideally we can migrate everything soon, including Node and PhetioObject as we proceed in phetsims/tasks#1050. Using the old pattern for mixin is acceptable until then.

Not sure if this is the right issue for the next idea: Now that Node supports TinyProperty, and has established support for passing in properties, PhET-iO support (linked elements & instrumentation), perhaps we should move enabledProperty directly to Node.

[jonathanolson]

Composition seems good for internal behavior, but not for the API, e.g.:

node.paintable.fill
node.transform.width
node.picker.pickable

// or

new Node( {
  transformOptions: {
    x: 5
  },
  paintableOptions: {
    fill: 'blue'
  },
  pickerOptions: {
    pickable: false
  }
} );

Those are all effectively properties of the node, NOT of components in the node that the users of the library should necessarily be aware of (even though, for instance, pickability is processed in a factored-out component called Picker).

I'll think on any way where the interface can be kept simple AND not duplicated, but I'm not aware of anything.

For instance, Paintable is currently only mixed into Path and Text which means we could create PaintableNode extends Node (unless we plan for future Paintable things to be non-Nodes).

That might be a special case where this could be considered, but but it wouldn't help in general.

[samreid]

I think we would maintain the API via forwarding methods, like:

get fill(){ return this.paintable.fill;}

I'm not opposed to duplication of these adapter-style methods because they are simple, clear, and unlikely to hide bugs.

[jonathanolson]

I'm not opposed to duplication of these adapter-style methods because they are simple, clear, and unlikely to hide bugs.

But any addition requires finding everywhere it's used and applying refactoring. It also adds runtime overhead, additional boilerplate code, etc.

[samreid]

Summary for April dev meeting:

1. @samreid confirmed that the proposed mixin pattern class FlowBox extends WidthSizable( HeightSizable( Node ) ) does not support autocomplete/highlighting/navigation.
2. @pixelzoom affirmed that composition is preferable to mixins when it is possible. @jonathanolson points out that we still want to maintain the same API even if we use composition, and I agree.

Proposed remaining work for this issue:

1. Convert anything using the old mixInto pattern to the new pattern? Poolable, AccessibleSlider, Leaf, AccessibleNumberSpinner, AccessibleValueHandler, ValueGestureControl. Who will convert these? What steps are necessary to convert?
2. Paintable is currently only mixed into Path and Text which means we could create PaintableNode extends Node?

[zepumph]

Today we noted that the new mixin pattern is a regression on Webstorm syntax highlighting for the supertypes. The main blocking issue is here is that Webstorm syntax highlighting.

JB and CK would like to have syntax highlighting, and don't use mixins very often, but don't feel like highlighting is a deal breaker to the new feature.

JB, CM: Messing up the supertype's api is a big problem to me.

SR: prefers patterns that support our primary IDE's workflow (syntax highlighting/autocomplete/etc)

JO: wants to use this new mixin pattern because besides webstorm's support it is preferable in every way, and factor's out a lot of code.

CM: If we need mixins so much, we are using the wrong language.

Possible ways forward:

• Find a way for our mixin pattern to support Webstorm
• Duplicate boilerplate in every spot we use the mixin (to support API forwarding to composition)
• Brainstorm other mixin patterns that would work better for Webstorm.
• See if @mixin jsdoc helps in supporting this.

[zepumph]

@samreid will look into using jsdoc, and @jonathanolson will investigate the others.

[samreid]

I investigated @mixin and @mixes in an isolated context, using this code example:

// Copyright 2014-2020, University of Colorado Boulder

/**
 * @mixes Administrator
 */
class Person {}

/**
 * @mixin
 */
class Administrator {
  goAdministrate() {}
}

/**
 * @mixin
 */
class Fisher {
  goFish() {}

  goDoSomething() {}
}

/**
 * @mixin
 */
class UnrelatedClass {
  goFish() {}
}

/**
 * @mixin
 */
class OtherType {
  goFish() {}

  goHide() {}
}

const p = new Person();
console.log( p );

const c = new UnrelatedClass();

p.goFish();

For autocomplete, the @mixes annotation doesn't seem to do anything. It wants to autocomplete anything in the file.

However, for navigation, it's a different story. If I put Person @mixes Fisher, navigation navigates goFish to the Fisher implementation. Likewise, if I put Person @mixes OtherType, then navigation navigates goFish to the OtherType implementation. Next I will try these experiments on our actual types.

[samreid]

I tried instantiating a new FlowBox at the bottom of FlowBox.js, and added @mixes WidthSizable like so:

/**
 * @mixes WidthSizable
 */
class FlowBox extends WidthSizable( HeightSizable( Node ) ) {

const f = new FlowBox();

It did not have any ideas about autocompleting preferredWidthProperty:

If I removed the memoize at the WidthSizable declaration, then it does better:

However, removing the @mixes did not disrupt this in any way.

In short, these JSDoc issues may help slightly.

[samreid]

I tried

/**
 * @mixes Poolable
 */
class ImageSVGDrawable extends ImageStatefulDrawable( SVGSelfDrawable ) {

and it was unable to autocomplete freeToPool:

[jonathanolson]

If I removed the memoize at the WidthSizable declaration, then it does better

So... potentially we could get rid of the memoize? It's designed to be better for memory/performance, but I haven't measured the effects.

[samreid]

CM: If we need mixins so much, we are using the wrong language.

An initial look at Typescript seems very promising, see phetsims/tasks#987. Perhaps we don't need to spend too much time finding the perfect long-term solution for this issue if we can migrate to typescript instead.

[jonathanolson]

Placing this on-hold, since we'd like to use Typescript for this if/when we move to it (phetsims/tasks#987).

[samreid]

As reported in phetsims/chipper#1049 (comment), I confirmed the pattern described in https://www.typescriptlang.org/docs/handbook/mixins.html has great IDE support.

[zepumph]

This issue is getting picked up and having traction over in phetsims/chipper#1127. @samreid found that the "new" pattern of mixin in typescript files (not .js files), fixes all the issues with IDE support and code completion. I'm going to start by converting ParallelDOM to the new mixin pattern over in phetsims/scenery#1159

[samreid]

The new pattern is working well, and we added better type checking for it in phetsims/tasks#1132, closing.