Can Multilink/DerivedProperty infer the callback parameter types? #395
Comments
|
Here is a snippet I made that gave me hope that Typescript itself is OK with provided less parameters for a callback. The issue seems to be about how the type inference is happening for Multilink. Furthermore Emitter is a great example of how we always provide the types, so perhaps that could be a worst-case. // Proof that callbacks support provided var args generally
class Hello<Parameters extends any[]> {
decide( t: Parameters, callback: ( ...params: Parameters ) => void ): void {
callback( ...t );
}
}
// No type error
new Hello<[ boolean, boolean ]>().decide( [ true, true ], ( something: boolean, somethingAgain: boolean ) => { console.log( something, somethingAgain ); } );
// Type error because of three parameters
new Hello<[ boolean, boolean ]>().decide( [ true, true ], ( something: boolean, somethingAgain: boolean, somethingAgain2: boolean ) => { console.log( something, somethingAgain, somethingAgain2 ); } );
// No type error
new Hello<[ boolean, boolean ]>().decide( [ true, true ], ( something: boolean ) => { console.log( something ); } ); |
|
Here is a snippet from stack overflow to help me see that we could create a tuple type of length N, but I didn't know if it would be possible to do that by inferring from a parameter type where all we know is that it extends https://stackoverflow.com/questions/52489261/typescript-can-i-define-an-n-length-tuple-type type Tuple<T, N extends number> = N extends N ? number extends N ? T[] : _TupleOf<T, N, []> : never;
type _TupleOf<T, N extends number, R extends unknown[]> = R['length'] extends N ? R : _TupleOf<T, N, [ T, ...R ]>;
type Tuple9<T> = Tuple<T, 9>;
type Board9x9<P> = Tuple9<Tuple9<P>>;
|
|
Here is our working copy trying to get a hard coded number of dependencies to work: Index: axon/js/Multilink.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Multilink.ts b/axon/js/Multilink.ts
--- a/axon/js/Multilink.ts (revision 14fcabf46dde58eb1b0db4ec12c606954b538af1)
+++ b/axon/js/Multilink.ts (date 1652132113304)
@@ -17,23 +17,32 @@
*/
import axon from './axon.js';
-import { MappedProperties } from './DerivedProperty.js';
import IReadOnlyProperty from './IReadOnlyProperty.js';
// constants
const GET_PROPERTY_VALUE = <T>( property: IReadOnlyProperty<T> ): T => property.get();
+
+// SO Gross
+type MappedProperties<T1, T2, T3, T4, T5> = T1 extends never ? [] :
+ T2 extends never ? [ IReadOnlyProperty<T1> ] :
+ T3 extends never ? [ IReadOnlyProperty<T1>, IReadOnlyProperty<T2> ] :
+ T4 extends never ? [ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3> ] :
+ T5 extends never ? [ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3>, IReadOnlyProperty<T4> ] :
+ [ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3>, IReadOnlyProperty<T4>, IReadOnlyProperty<T5> ];
+
+
const valuesOfProperties = <Parameters extends any[]>( dependencies: MappedProperties<Parameters> ): Parameters => {
// Typescript can't figure out that the map gets us back to the Parameters type
return dependencies.map( GET_PROPERTY_VALUE ) as Parameters;
};
// Type of a derivation function, that takes the typed parameters (as a tuple type)
-type Callback<Parameters extends any[]> = ( ...params: Parameters ) => void;
+type Callback<T1, T2, T3, T4, T5> = ( p1: T1, p2: T2, p3: T3, p4: T4, p5: T5 ) => void;
-export default class Multilink<Parameters extends any[]> {
+export default class Multilink<T1 = never, T2 = never, T3 = never, T4 = never, T5 = never> {
- private dependencies: MappedProperties<Parameters> | null;
+ private dependencies: MappedProperties<T1, T2, T3, T4, T5> | null;
private dependencyListeners: Map<IReadOnlyProperty<any>, () => void>;
private isDisposed?: boolean;
@@ -43,7 +52,7 @@
* @param callback function that expects args in the same order as dependencies
* @param [lazy] Optional parameter that can be set to true if this should be a lazy multilink (no immediate callback)
*/
- constructor( dependencies: MappedProperties<Parameters>, callback: Callback<Parameters>, lazy?: boolean ) {
+ constructor( dependencies: MappedProperties<T1, T2, T3, T4, T5>, callback: Callback<T1, T2, T3, T4, T5>, lazy?: boolean ) {
this.dependencies = dependencies;
Index: ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts b/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts
--- a/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts (revision a1e0d01faa526ebef8ca2c626cd943ce08527af4)
+++ b/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts (date 1652132113311)
@@ -19,6 +19,7 @@
import RatioDescriber from '../../common/view/describers/RatioDescriber.js';
import IReadOnlyProperty from '../../../../axon/js/IReadOnlyProperty.js';
import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
+import Multilink from '../../../../axon/js/Multilink.js';
type RatioToChallengeNameMap = Map<number, { lowercase: string; capitalized: string }>;
@@ -81,14 +82,36 @@
this.ratioToChallengeNameMap = ratioToChallengeNameMap;
// This derivedProperty is already dependent on all other dependencies for getStateOfSimString
- Property.multilink( [
+ const x = new Multilink( [
+
+ /*
+
+ With inProportionProperty commented out, these 4 have this error:
+ TS2322: Type 'Property ' is not assignable to type 'never'.
+ */
+
targetRatioProperty,
tickMarkViewProperty,
ratioTupleProperty,
ratioFitnessProperty,
- inProportionProperty
- ], ( currentTargetRatio: number, tickMarkView: TickMarkView, currentTuple: RAPRatioTuple, fitness: number, inProportion: boolean ) => {
+
+ /*
+ Works great with this commented in if we hard code MappedProperties to just work for 5 values, but the
+ current <=5 varargs implementation breaks this, but we couldn't get it to take less parameters just yet, it
+ thought it was all `never`. Commented in it says:
+ S2322: Type 'IReadOnlyProperty<boolean>' is not assignable to type 'IReadOnlyProperty<false> | IReadOnlyProperty<true>'.
+ Type 'IReadOnlyProperty<boolean>' is not assignable to type 'IReadOnlyProperty<false>'.
+ Type 'boolean' is not assignable to type 'false'.
+ */
+ // inProportionProperty
+ ],
+
+ // These have the correct typing! YAY! (number, TickMarkView,RAPRatioTuple,number,boolean)
+ ( p1,p2, p3,p4,p5 ) => {
+
+ // Type error, it knows it is a number
+ const x: string = p1;
stateOfSimNode.innerContent = this.getStateOfSim();
leftHandBullet.innerContent = this.getLeftHandState();
rightHandBullet.innerContent = this.getRightHandState();
|
|
@samreid and I left this investigation thinking it would be good to create a minimal case and see if any typescript experts can assist with this challenge. Likely exactly what I want is impossible, something like this: type Callback<Parameters, N> = N extends 0? Parameters : ( (...params: Parameters[0:N] ) => void ) | Callback<Parameters,N-1>; |
|
There is an interesting example here: https://stackoverflow.com/a/70568063/1009071 function h<T extends any[] >(x: T): T;
h([1, 2, "hello"]); // => (string | number)[]
function h<T extends any[] | []>(x: T): T;
h([1, 2, "hello"]); // => [number, number, string]I don't know how it works, but I tried putting |
|
This patch has correct inference of the type and supports an arbitrary number of parameters in the callback: Index: main/axon/js/DerivedProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/DerivedProperty.ts b/main/axon/js/DerivedProperty.ts
--- a/main/axon/js/DerivedProperty.ts (revision 2ee1223d582611a473c6363b303af22771310a8a)
+++ b/main/axon/js/DerivedProperty.ts (date 1652153246683)
@@ -27,7 +27,7 @@
export type DerivedPropertyOptions<T> = SelfOptions & PropertyOptions<T>;
// Maps tuples/arrays from T => IReadOnlyProperty<T>
-export type MappedProperties<Parameters extends any[]> = {
+export type MappedProperties<Parameters> = {
[K in keyof Parameters]: IReadOnlyProperty<Parameters[K]>;
};
Index: main/axon/js/Property.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/Property.ts b/main/axon/js/Property.ts
--- a/main/axon/js/Property.ts (revision 2ee1223d582611a473c6363b303af22771310a8a)
+++ b/main/axon/js/Property.ts (date 1652153835891)
@@ -15,7 +15,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
-import Multilink from './Multilink.js';
+import Multilink, { Callback } from './Multilink.js';
import propertyStateHandlerSingleton from './propertyStateHandlerSingleton.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import TinyProperty from './TinyProperty.js';
@@ -507,7 +507,7 @@
* @param properties
* @param listener function that takes values from the properties and returns nothing
*/
- static multilink<Parameters extends any[]>( properties: MappedProperties<Parameters>, listener: ( ...params: Parameters ) => void ): Multilink<Parameters> {
+ static multilink<Parameters extends readonly any[]>( properties: MappedProperties<Parameters>, listener: Callback<Parameters> ): Multilink<Parameters> {
return new Multilink( properties, listener, false );
}
Index: main/axon/js/Multilink.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/Multilink.ts b/main/axon/js/Multilink.ts
--- a/main/axon/js/Multilink.ts (revision 2ee1223d582611a473c6363b303af22771310a8a)
+++ b/main/axon/js/Multilink.ts (date 1652153969938)
@@ -23,15 +23,24 @@
// constants
const GET_PROPERTY_VALUE = <T>( property: IReadOnlyProperty<T> ): T => property.get();
-const valuesOfProperties = <Parameters extends any[]>( dependencies: MappedProperties<Parameters> ): Parameters => {
+const valuesOfProperties = <Parameters extends readonly any[]>( dependencies: MappedProperties<Parameters> ): Parameters => {
// Typescript can't figure out that the map gets us back to the Parameters type
- return dependencies.map( GET_PROPERTY_VALUE ) as Parameters;
+
+ // @ts-ignore
+ return dependencies.map( GET_PROPERTY_VALUE );
};
// Type of a derivation function, that takes the typed parameters (as a tuple type)
-type Callback<Parameters extends any[]> = ( ...params: Parameters ) => void;
+export type Callback<Parameters extends Readonly<any[]>> =
+ ( () => void ) |
+ ( ( p0: Parameters[0] ) => void ) |
+ ( ( p0: Parameters[0], p1: Parameters[1] ) => void ) |
+ ( ( p0: Parameters[0], p1: Parameters[1], p2: Parameters[2] ) => void ) |
+ ( ( p0: Parameters[0], p1: Parameters[1], p2: Parameters[2], p3: Parameters[3] ) => void ) |
+ ( ( p0: Parameters[0], p1: Parameters[1], p2: Parameters[2], p3: Parameters[3], p4: Parameters[4] ) => void ) |
+ ( ( p0: Parameters[0], p1: Parameters[1], p2: Parameters[2], p3: Parameters[3], p4: Parameters[4], p5: Parameters[5] ) => void );
-export default class Multilink<Parameters extends any[]> {
+export default class Multilink<Parameters extends readonly any[]> {
private dependencies: MappedProperties<Parameters> | null;
private dependencyListeners: Map<IReadOnlyProperty<any>, () => void>;
@@ -59,6 +68,8 @@
// don't call listener if this Multilink has been disposed, see https://github.com/phetsims/axon/issues/192
if ( !this.isDisposed ) {
+
+ // @ts-ignore
callback( ...valuesOfProperties( dependencies ) );
}
};
@@ -74,6 +85,8 @@
// Send initial call back but only if we are non-lazy
if ( !lazy ) {
+
+ // @ts-ignore
callback( ...valuesOfProperties( dependencies ) );
}
Index: main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts b/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts
--- a/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts (revision d5730599aec919ca313f9aacefe85b5ecef0f388)
+++ b/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts (date 1652154024986)
@@ -87,7 +87,7 @@
ratioTupleProperty,
ratioFitnessProperty,
inProportionProperty
- ], ( currentTargetRatio: number, tickMarkView: TickMarkView, currentTuple: RAPRatioTuple, fitness: number, inProportion: boolean ) => {
+ ] as const, ( currentTargetRatio: number, tickMarkView: TickMarkView, currentTuple: RAPRatioTuple, fitness: number, inProportion: boolean ) => {
stateOfSimNode.innerContent = this.getStateOfSim();
leftHandBullet.innerContent = this.getLeftHandState();
The cost of this approach:
Before starting on this issue, I thought it would be too demanding to require developers to put Where would it be acceptable to leave off the @zepumph can we discuss this and see if it gets us closer to adopting the no-unused-variables rule? |
|
It was helpful to just refresh myself on const assertions for this https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#const-assertions |
|
I personally really like the approach of having |
|
All the ts-ignores seem to be because Array.map doesn't preserve the tuple-ness of Parameters. I am getting closer, but https://stackoverflow.com/questions/57913193/how-to-use-array-map-with-tuples-in-typescript is really helping me understand. I'll report back. |
|
Ahhh, perhaps "Mapped Tuples" in https://stackoverflow.com/questions/51672504/how-to-map-a-tuple-to-another-tuple-type-in-typescript-3-0. |
|
I wanted to chime in quickly that while |
|
#395 (comment) is making me optimistic that we can potentially do either, since it is mostly about converting between tuples of the same length, but different types. This is what we want to get from MappedProperties to Parameters, using Parameters for the callback and other items in the file. |
|
@samreid, I'm not sure your solution or my further investigation here will work in it's current form. While it successfully knows when you pass it a good callback, it doesn't infer the parameter types of a parameter that doesn't have a type added to the callback:
I still believe that the |
|
Yes, in my investigation, it was necessary to specify the types of the callback parameters, like |
|
This patch has the following behavior: Index: main/axon/js/Multilink.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/Multilink.ts b/main/axon/js/Multilink.ts
--- a/main/axon/js/Multilink.ts (revision 13baf1e5dd482848b7c28854a3992fe51706ef01)
+++ b/main/axon/js/Multilink.ts (date 1652214621975)
@@ -23,17 +23,22 @@
// constants
const GET_PROPERTY_VALUE = <T>( property: IReadOnlyProperty<T> ): T => property.get();
-const valuesOfProperties = <Parameters extends any[]>( dependencies: MappedProperties<Parameters> ): Parameters => {
+const valuesOfProperties = <Parameters extends readonly any[]>( dependencies: MappedProperties<Parameters> ): Parameters => {
// Typescript can't figure out that the map gets us back to the Parameters type
- return dependencies.map( GET_PROPERTY_VALUE ) as Parameters;
+
+ // @ts-ignore
+ return dependencies.map( GET_PROPERTY_VALUE );
};
-// Type of a derivation function, that takes the typed parameters (as a tuple type)
-type Callback<Parameters extends any[]> = ( ...params: Parameters ) => void;
+type Dependencies<T1, T2, T3, T4, T5> = Readonly<[ IReadOnlyProperty<T1> ]> |
+ Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2> ]> |
+ Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3> ]> |
+ Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3>, IReadOnlyProperty<T4> ]> |
+ Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3>, IReadOnlyProperty<T4>, IReadOnlyProperty<T5> ]>;
-export default class Multilink<Parameters extends any[]> {
+export default class Multilink<T1, T2, T3, T4, T5> {
- private dependencies: MappedProperties<Parameters> | null;
+ private dependencies: Dependencies<T1, T2, T3, T4, T5> | null;
private dependencyListeners: Map<IReadOnlyProperty<any>, () => void>;
private isDisposed?: boolean;
@@ -43,7 +48,12 @@
* @param callback function that expects args in the same order as dependencies
* @param [lazy] Optional parameter that can be set to true if this should be a lazy multilink (no immediate callback)
*/
- constructor( dependencies: MappedProperties<Parameters>, callback: Callback<Parameters>, lazy?: boolean ) {
+ constructor( dependencies: Readonly<[ IReadOnlyProperty<T1> ]>, callback: ( ...params: [ T1 ] ) => void, lazy?: boolean ) ;
+ constructor( dependencies: Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2> ]>, callback: ( ...params: [ T1, T2 ] ) => void, lazy?: boolean ) ;
+ constructor( dependencies: Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3> ]>, callback: ( ...params: [ T1, T2, T3 ] ) => void, lazy?: boolean ) ;
+ constructor( dependencies: Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3>, IReadOnlyProperty<T4> ]>, callback: ( ...params: [ T1, T2, T3, T4 ] ) => void, lazy?: boolean ) ;
+ constructor( dependencies: Readonly<[ IReadOnlyProperty<T1>, IReadOnlyProperty<T2>, IReadOnlyProperty<T3>, IReadOnlyProperty<T4>, IReadOnlyProperty<T5> ]>, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void, lazy?: boolean ) ;
+ constructor( dependencies: Dependencies<T1, T2, T3, T4, T5>, callback: ( ...params: [ T1, T2, T3, T4, T5 ] ) => void, lazy?: boolean ) {
this.dependencies = dependencies;
@@ -59,6 +69,8 @@
// don't call listener if this Multilink has been disposed, see https://github.com/phetsims/axon/issues/192
if ( !this.isDisposed ) {
+
+ // @ts-ignore
callback( ...valuesOfProperties( dependencies ) );
}
};
@@ -74,6 +86,8 @@
// Send initial call back but only if we are non-lazy
if ( !lazy ) {
+
+ // @ts-ignore
callback( ...valuesOfProperties( dependencies ) );
}
@@ -84,7 +98,7 @@
/**
* Returns dependencies that are guaranteed to be defined internally.
*/
- private get definedDependencies(): MappedProperties<Parameters> {
+ private get definedDependencies(): Dependencies<T1, T2, T3, T4, T5> {
assert && assert( this.dependencies !== null, 'Dependencies should be defined, has this Property been disposed?' );
return this.dependencies!;
}
Index: main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts b/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts
--- a/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts (revision 1dfcd906762fa350b98325191ad3d28fe34da75e)
+++ b/main/ratio-and-proportion/js/discover/view/DiscoverScreenSummaryNode.ts (date 1652214699410)
@@ -19,6 +19,7 @@
import RatioDescriber from '../../common/view/describers/RatioDescriber.js';
import IReadOnlyProperty from '../../../../axon/js/IReadOnlyProperty.js';
import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
+import Multilink from '../../../../axon/js/Multilink.js';
type RatioToChallengeNameMap = Map<number, { lowercase: string; capitalized: string }>;
@@ -81,13 +82,13 @@
this.ratioToChallengeNameMap = ratioToChallengeNameMap;
// This derivedProperty is already dependent on all other dependencies for getStateOfSimString
- Property.multilink( [
+ const x = new Multilink( [
targetRatioProperty,
tickMarkViewProperty,
ratioTupleProperty,
ratioFitnessProperty,
inProportionProperty
- ], ( currentTargetRatio: number, tickMarkView: TickMarkView, currentTuple: RAPRatioTuple, fitness: number, inProportion: boolean ) => {
+ ], ( targetRatio, tickMarkView ) => {
stateOfSimNode.innerContent = this.getStateOfSim();
leftHandBullet.innerContent = this.getLeftHandState();
No need to specify
|
|
Very fun! Good on ya. I just used this code snippet to note that area model algebra has a multilink with 9 dependencies. I'm still running to get a grand total, but I could totally see a case where some crazy multilink needed to link to 50 Properties. Is that a deal breaker? Index: js/Multilink.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Multilink.ts b/js/Multilink.ts
--- a/js/Multilink.ts (revision 13baf1e5dd482848b7c28854a3992fe51706ef01)
+++ b/js/Multilink.ts (date 1652217482913)
@@ -20,6 +20,7 @@
import { MappedProperties } from './DerivedProperty.js';
import IReadOnlyProperty from './IReadOnlyProperty.js';
+let bigNum = 0;
// constants
const GET_PROPERTY_VALUE = <T>( property: IReadOnlyProperty<T> ): T => property.get();
@@ -45,6 +46,11 @@
*/
constructor( dependencies: MappedProperties<Parameters>, callback: Callback<Parameters>, lazy?: boolean ) {
+ if ( dependencies.length > bigNum ) {
+ bigNum = dependencies.length;
+ console.log( bigNum );
+ }
+
this.dependencies = dependencies;
assert && assert( dependencies.every( _.identity ), 'dependencies should all be truthy' ); |
|
Expression Exchange has a multilink with 10 listeners. |
|
Maybe we should target 10 and cases that need more can use multiple multilinks, or use a ts-ignore? |
|
I think we can update your patch to use a
What about a runtime assertion for that number, and whenever anyone hits it, we just increase the number? And start it at 15 and chances are good that it will be a while before we do hit it. assert && assert( dependencies.length <15, 'type information is only available for less than 15 dependencies, please update the types for Callback and dependencies to continue' ); |
|
I made only minor improvements I believe. I think that we could use this, or perhaps expand it to factor out some of the duplicated code. The internet seem to be in alignment that the way to type a tuple from an array is the intersect it with
I didn't get to a patch, but I put this into the usages of Dependencies and it worked well. |
|
Some extensive changes on the way for Multilink. Summarizing, and the benefits are also described in #395 (comment)
Local tests are going well. Fuzz tests OK. Snapshot comparison looks mostly good:
Also noting to @jonathanolson that it was easy enough to use different ports for this test. Next steps will be:
I wrote to slack:
|
|
Latest checklist in case it gets hidden by commits: #395 (comment) UPDATE: I finished the checkboxes assigned to me, over to @zepumph for review and seeing if the types can be simplified without making other sacrifices. |
…DerivedProperty, see phetsims/axon#395
|
Fixed some aqua imports above |
|
@samreid, I see a couple of changes that may have broken the PhET-iO API. For example phetsims/joist@4128a84. Can you recommend how to proceed? I wasn't sure why we change PhET-iO metadata as part of this issue, but I also don't want to just revert things before discussing with you. |
|
I'm really not sure about any way to improve this. I played around with a couple of changes, but it cascaded to a real headache. Thanks for doing all of this. It is awesome! |
|
Thanks, closing. |
…DerivedProperty, see phetsims/axon#395
@samreid and I ran into this while trying to figure out how to get rid of unused parameters in callbacks for DerivedProperty and Multilink (in phetsims/chipper#1230). The current behavior is that you must provide the right types for the callback, because that is how the Parameter types are inferred. They aren't seeming to be inferred from the dependencies arrays (unfortunately).
Tagging @jonathanolson because he was the original converter to typescript.
Ideally we could specify any USED parameters needed for the callback, and they would have correct type information gathered from the list of dependencies provided.
The text was updated successfully, but these errors were encountered: