TypeScript mixins

[samreid]

Related to #1049, I set up a model of mixins following the new approach we have been using, which matches the recommended style in TypeScript https://www.typescriptlang.org/docs/handbook/mixins.html

Here's a patch that demonstrates this pattern using Node, PhetioObject and a ParallelDOM mixin, and a Node subtype of Panel:

Index: js/testts/MyNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/testts/MyNode.ts b/js/testts/MyNode.ts
new file mode 100644
--- /dev/null	(date 1634915520676)
+++ b/js/testts/MyNode.ts	(date 1634915520676)
@@ -0,0 +1,10 @@
+// Copyright 2021, University of Colorado Boulder
+
+import MyParallelDOM from './MyParallelDOM.js';
+import MyPhetioObject from './MyPhetioObject.js';
+
+class MyNode extends MyParallelDOM( MyPhetioObject ) {
+
+}
+
+export default MyNode;
\ No newline at end of file
Index: js/testts/MyPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/testts/MyPanel.ts b/js/testts/MyPanel.ts
new file mode 100644
--- /dev/null	(date 1634916399665)
+++ b/js/testts/MyPanel.ts	(date 1634916399665)
@@ -0,0 +1,16 @@
+// Copyright 2021, University of Colorado Boulder
+
+import MyNode from './MyNode.js';
+
+class MyPanel extends MyNode {
+  constructor() {
+    super();
+
+    console.log( this.tandem );
+    console.log( this.divName );
+    // console.log( this.fakeAttribute );
+    this.focus();
+  }
+}
+
+export default MyPanel;
\ No newline at end of file
Index: js/testts/MyParallelDOM.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/testts/MyParallelDOM.ts b/js/testts/MyParallelDOM.ts
new file mode 100644
--- /dev/null	(date 1634916237068)
+++ b/js/testts/MyParallelDOM.ts	(date 1634916237068)
@@ -0,0 +1,21 @@
+// Copyright 2021, University of Colorado Boulder
+
+type Constructor = new ( ...args: any[] ) => {};
+
+function MyParallelDOM<TBase extends Constructor>( Base: TBase ) {
+  return class extends Base {
+    divName: string;
+
+    constructor( ...args: any[] ) {
+      super();
+      this.divName = 'my div';
+    }
+
+    // @public
+    focus() {
+      console.log( 'focus' );
+    }
+  };
+}
+
+export default MyParallelDOM;
\ No newline at end of file
Index: js/testts/MyPhetioObject.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/testts/MyPhetioObject.ts b/js/testts/MyPhetioObject.ts
new file mode 100644
--- /dev/null	(date 1634916307026)
+++ b/js/testts/MyPhetioObject.ts	(date 1634916307026)
@@ -0,0 +1,11 @@
+// Copyright 2021, University of Colorado Boulder
+
+class MyPhetioObject {
+  tandem: string;
+
+  constructor() {
+    this.tandem = 'hello!';
+  }
+}
+
+export default MyPhetioObject;
\ No newline at end of file

The usage site looks like this:

// Copyright 2021, University of Colorado Boulder

import MyNode from './MyNode.js';

class MyPanel extends MyNode {
  constructor() {
    super();

    console.log( this.tandem );  // Declared in the parent type
    console.log( this.divName ); // Declared in the mixin
    this.focus();                // Declared in the mixin
    // console.log( this.fakeAttribute ); // Not declared anywhere.  Fails type check
  }
}

export default MyPanel;

and has correct type checking and navigation in the IDE and correct type checking from tsc. So this seems like a good pattern we can continue using.

The mixin declaration looks like this:

// Copyright 2021, University of Colorado Boulder

type Constructor = new ( ...args: any[] ) => {};

function MyParallelDOM<TBase extends Constructor>( Base: TBase ) {
  return class extends Base {
    divName: string;

    constructor( ...args: any[] ) {
      super();
      this.divName = 'my div';
    }

    // @public
    focus() {
      console.log( 'focus' );
    }
  };
}

export default MyParallelDOM;

If writing these files in *.js, they are inferred as any and while it passes the type checking, it also doesn't help. So we would need to use *.ts for those, or investigate JSDoc to get it to not infer as any if that is possible.

[zepumph]

This issue is largely an extension of phetsims/phet-info#143. Given your findings above, I feel like the most prudent thing to do is to convert ParallelDOM to a new-style mixin. This should be simple because it only has a single usage.

This will remove 40 compile errors in ratio and proportion

[samreid]

@jonathanolson also identified some constraints on TypeScript mixins not being able to use private/protected members, but I'm having trouble understanding/replicating that problem.

[samreid]

Notes from my discussion with @jonathanolson

• We looked at the legacy mixin pattern described in https://www.typescriptlang.org/docs/handbook/mixins.html and found that it may accomplish the typing correctly, but has the constraint that the mixin types cannot have constructors. We may be able to work around this via initialize() methods, but we didn’t like that so much.
• We tried our new mixin pattern, and it works well, but has the following constraints
  • You cannot specify a constrained base type with private attributes - casts are needed inside mixin code. You can use a runtime check for checking the base type is the right type
  • We tried using # for marking private things, but that seems at odds with typescript private/protected/public, and doesn’t support protected, and also emitted less-optimized code.
  • We committed the latest version of this in wilder.
  • We still haven’t checked how to get the constructor args
  • Now that memoize is typed (in wilder), we don’t need an additional interface declaration that has to be maintained in parallel.
• We also discussed that in some cases, it may be preferable to avoid mixins. For instance, if we thought Paintable would never be used elsewhere, we could convert to PaintableNode extends Node, then Path and Text extend PaintableNode.
• @jonathanolson looked at plucking arguments from the constructor and trying to detect when options is provided, or whether we would require config for cases like these.

[zepumph]

@jonathanolson and I talked briefly about phetsims/scenery@89b51f0 and feel good about the change.

[jonathanolson]

Constructor arguments work, as long as we parse them out from ...args: any[]. Passing a "number of parameters" or "where is the options object" parameter into a mixin work well (demo in Mixable.ts). We can pass different params to the supertype, so basically it's fully workable (although a bit inconvenient).

Mixins/traits with generic type attributes are working nicely.

I added a demo of Poolable to Mixable.ts, it is able to have an impressive level of strict typing (createFromPool/etc. get typed arguments and name hints in the IDE).

[pixelzoom]

Has the pattern recommended in Programming Typescript (p. 101) been evaluated? The book claims that its “easy” to implement mixins/traits in TS, then has an extended example. There’s no mention of problems with private and protected. I’m not very familiar with how/where/why PhET is using mixins/traits (my preference is to avoid them), so I don’t know how well the book’s approach matches PhET’s needs.

EDIT: The book's example looks similar to https://www.typescriptlang.org/docs/handbook/mixins.html. The example in that link does have this comment about private/protected:

    // Mixins may not declare private/protected properties
    // however, you can use ES2020 private fields

[samreid]

@jonathanolson can you please review the following 3rd party libraries and evaluate whether they would be a good match for our project?

https://github.com/tannerntannern/ts-mixer
https://github.com/fasttime/Polytype
https://github.com/bcherny/typed-trait/blob/master/src/index.ts

[samreid]

@jonathanolson and I discussed this today and feel like steps like these would be good:

• We aren't excited about ts-mixer since it doesn't correctly do instanceof checks
• Polytype has a low community support and looks like it is based on Proxy. It may therefore have performance concerns.
• We don't remember why we avoided the legacy "alternative" mixin pattern in the handbook: https://www.typescriptlang.org/docs/handbook/mixins.html. Maybe it is worth re-investigating.

The main problems with the approach in wilder at the moment are:

• It doesn't know the type in the mixin constructor: ( this as unknown as Node ).addChild( node );. This could be remedied by putting in the extends parameter, but not possible in this case if Node has private members.
• Doesn't support private or protected attributes in the mixin. Or we could try # to make these private, but there are downsides to that. Maybe underscore is preferable and we would keep it private by convention.

We could always start with the wilder pattern then switch to another pattern if we find a better one. We can't apply it in common code until common code is using TypeScript. Until then, we have the d.ts workaround and this new Image overrides workaround.

• We discussed that mixins aren't always the best solution, sometimes other approaches are better. Mixins are not appropriate if you just want to have code in a different file and mix into a single core type. But we do need to support mixins for some things like Imageable.
• We discussed using composition instead of mixins, but this has a maintainability burden and performance overhead--changing one thing in the mixin requires visiting all of the mixed sites.

[samreid]

I tested changing Imageable like so:

type Constructor = new ( ...args: any[] ) => {};
/**
 * @param {constructor} type
 */
const Imageable = <Base extends Constructor>( type: Base ) => {
  return class extends type {

Then I was able to create an Image, and access elements from Node, Image and Imageable, and bodyNode instanceof Node evaluated to true. This pattern seems like it is working really well, and we should just get used to the constraints about private members and/or this-casting in the constructor.

Unassigning until we have TypeScript in common code.

[pixelzoom]

Can the same pattern that was used for Imageable also be used for ParallelDOM mixin? Geometric Optics uses fields and methods that Node gets from ParallelDOM, and I'm currently resorting to @ts-ignore.

[samreid]

It seems appropriate to use ts-ignore until we enable TypeScript in common code. Self-unassigning for now.

[zepumph]

Hello mixin issue! We have not updated this in quite some time, but lots of progress has been made. @zepumph took the lead on phetsims/scenery#1340, and @jonathanolson has converted Imageable, Paintable, and Poolable to typescript in the last quarter. We have established a pattern of how we want Typescript mixins to look (when possible) in our project, and documented it in https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md#mixin-and-trait.

Tagging @jessegreenberg and @jonathanolson in case they have anything else to provide here. I'm going to close, but feel free to reopen if there are open questions or concerns that have not been addressed.