phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

DerivedProperty and Multilink types cannot be inferred when dependencies are computed. #417

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

DerivedProperty and Multilink have a similar problem when their dependencies are computed. For example:

const dependencies = atoms.map( atom => atom.positionProperty );
const multilink = new Multilink( dependencies, () => { ... } );

The usage of dependencies on the 2nd line results in TS error:

TS2769: No overload matches this call.   The last overload gave the following error.     Argument of type 'DerivedProperty<number, number, any, any, any, any, any, any, any, any, any, any, any, any, any, any>[]' is not assignable to parameter of type 'Dependencies<unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown, unknown>'.       Type 'DerivedProperty<number, number, any, any, any, any, any, any, any, any, any, any, any, any, any, any>[]' is not assignable to type 'readonly [ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP, ROP<...>]'.         Target requires 15 element(s) but source may have fewer.

I've also tried this, which results in the same error:

const dependencies: TReadOnlyProperty<Vector2> = atoms.map( atom => atom.positionProperty );
const multilink = new Multilink( dependencies, () => { ... } );

DerivedProperty and Multilink have multiple constructors, and the number of values in the first arg (dependencies) is used to infer type. When dependencies is computed, that hack doesn't work. And I can't explicitly specify the type, because I don't know how many dependencies there will be - in the example above, it varies based on the number of atoms.

Thoughts on how to address this? This pattern is used in several places in Equality Explorer, which I'm in the process of converting to TS in https://github.com/phetsims/equality-explorer/issues/186.

pixelzoom commented 2 years ago

The current implementations of DerivedProperty and Multilink are limited to 15 dependencies. In the above example, I could certainly have more than 15 atoms.

pixelzoom commented 2 years ago

For 2 real-world examples of this problem, see equality-explorer EquationNode.ts, and the multiple // @ts-ignore comments related to relationalOperatorMultilink and termsMultilink.

samreid commented 2 years ago

This kind of problem is best solved with multilinkAny which returns type UnknownMultilink. Here is a working patch which demonstrates its use in EquationNode.ts:

```diff Index: js/common/view/EquationNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/EquationNode.ts b/js/common/view/EquationNode.ts --- a/js/common/view/EquationNode.ts (revision fd32e99d6c9827cc0af8ed5f028754ac5aed5309) +++ b/js/common/view/EquationNode.ts (date 1663383156967) @@ -8,7 +8,7 @@ * @author Chris Malley (PixelZoom, Inc.) */ -import Multilink from '../../../../axon/js/Multilink.js'; +import Multilink, { UnknownMultilink } from '../../../../axon/js/Multilink.js'; import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import optionize from '../../../../phet-core/js/optionize.js'; import Fraction from '../../../../phetcommon/js/model/Fraction.js'; @@ -136,10 +136,8 @@ updateLayout(); }; - // @ts-ignore TODO https://github.com/phetsims/equality-explorer/issues/186 - let relationalOperatorMultilink; // {Multilink|undefined} defined for dynamic equations - // @ts-ignore TODO https://github.com/phetsims/equality-explorer/issues/186 - let termsMultilink; // {Multilink|undefined} defined for dynamic equations + let relationalOperatorMultilink: UnknownMultilink | undefined; // {Multilink|undefined} defined for dynamic equations + let termsMultilink: UnknownMultilink | undefined; // {Multilink|undefined} defined for dynamic equations if ( options.updateEnabled ) { // The equation needs to be dynamically updated. @@ -157,10 +155,8 @@ } ); // dispose required - // @ts-ignore TODO https://github.com/phetsims/equality-explorer/issues/186 - relationalOperatorMultilink = new Multilink( relationalOperatorDependencies, updateRelationalOperator ); - // @ts-ignore TODO https://github.com/phetsims/equality-explorer/issues/186 - termsMultilink = new Multilink( termDependencies, updateTerms ); + relationalOperatorMultilink = Multilink.multilinkAny( relationalOperatorDependencies, updateRelationalOperator ); + termsMultilink = Multilink.multilinkAny( termDependencies, updateTerms ); } else { @@ -170,9 +166,7 @@ } this.disposeEquationNode = () => { - // @ts-ignore TODO https://github.com/phetsims/equality-explorer/issues/186 relationalOperatorMultilink && relationalOperatorMultilink.dispose(); - // @ts-ignore TODO https://github.com/phetsims/equality-explorer/issues/186 termsMultilink && termsMultilink.dispose(); }; ```

For DerivedProperty, the analog is deriveAny which returns UnknownDerivedProperty.

pixelzoom commented 2 years ago

Great thanks. I had no idea that Multilink.multilinkAny or UnknownMultilink existed. The former has no documentation, the latter has inadequate documentation, see https://github.com/phetsims/axon/issues/418.