Closed zepumph closed 2 years ago
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.
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 any[]
.
https://stackoverflow.com/questions/52489261/typescript-can-i-define-an-n-length-tuple-type
= Tuple9
Here is our working copy trying to get a hard coded number of dependencies to work:
@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 | []
a few places in Multilink but couldn't get it working.
This patch has correct inference of the type and supports an arbitrary number of parameters in the callback:
The cost of this approach:
as const
on the array. Before starting on this issue, I thought it would be too demanding to require developers to put as const
at multilink (and potentially derivedproperty) sites. However, as mentioned in https://stackoverflow.com/a/63322898/1009071 it does seem part of the language to infer (T1|T2|T3)[]
from a plain array, so I'm more inclined to advocate to the team to use as const
in cases like these.
Where would it be acceptable to leave off the as const
? If there are 0 parameters in the callback, you can get away with it. Also, if there is only one element in the array and 1 parameter in the callback, it will still type check without as const
, but it won't shield you from accidentally putting too many parameters in the callback (all with the same type).
@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 as const
as the typing for Multilink and DerivedProperty dependencies. It is definitely a lot of of changing, but it also makes sense that that is how you infer the parameter types for the instance. It should not come from the callback. I'll take a look at the patch and report back.
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 as const
tuples seem natural, we haven't completely ruled out the possibility of using types like Multilink<T1=never, T2=never>
, etc (though it wasn't trivial to progress in that direction).
https://github.com/phetsims/axon/issues/395#issuecomment-1122642720 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 as const
method is the way forward, perhaps since it is the way that makes the most sense to me and that I can trust. What do you think about this issue though?
Yes, in my investigation, it was necessary to specify the types of the callback parameters, like p1: number
. While that may be an acceptable cost, I'd like to spend more time on this issue to see if that can be inferred.
This patch has the following behavior:
as const
Next steps:
Property.multilink
accordinglyVery 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?
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 Tuple
type instead of duplicating all those IReadOnlyProperties. I'll work on that hopefully today.
Maybe we should target 10 and cases that need more can use multiple multilinks, or use a ts-ignore?
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 { length: X }
for an X length Tuple.
type MapToPropertiesTuple<TItem extends readonly any[], TLength extends number = TItem['length']> = [ ...MappedProperties<TItem> ] & { length: TLength };
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 https://github.com/phetsims/axon/issues/395#issuecomment-1122835967
unmultilink
, please use type UnknownMultilink
.Property.multilinkAny
. Note this does not give callback values.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:
significant changes for Multilink will be committed momentarily. Changes in 83 files across 20 repos. Notes in https://github.com/phetsims/axon/issues/395#issuecomment-1126792124
Main usage changes:
- Do not specify the Multilink generic type parameters, let it infer them.
- Do not specify the multilink callback parameters, let it infer them.
- If you have an unknown array of properties to observe, use Property.multilinkAny instead. Similar changes for DerivedProperty coming next.
All changes pushed.
Multilink and DerivedProperty complete. From https://github.com/phetsims/axon/issues/395#issuecomment-1126792124, the remaining steps are:
Next steps will be:
CT is showing errors like:
Dependencies should be defined, has this Property been disposed?
and
energy-skate-park-basics : fuzz : built-phet-io
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1652655453957/energy-skate-park-basics/build/phet-io/energy-skate-park-basics_all_phet-io.html?continuousTest=%7B%22test%22%3A%5B%22energy-skate-park-basics%22%2C%22fuzz%22%2C%22built-phet-io%22%5D%2C%22snapshotName%22%3A%22snapshot-1652655453957%22%2C%22timestamp%22%3A1652663069319%7D&fuzz&memoryLimit=1000&phetioStandalone
Query: fuzz&memoryLimit=1000&phetioStandalone
Uncaught TypeError: Cannot read properties of null (reading 'map')
TypeError: Cannot read properties of null (reading 'map')
at c (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1652655453957/energy-skate-park-basics/build/phet-io/energy-skate-park-basics_all_phet-io.html?continuousTest=%7B%22test%22%3A%5B%22energy-skate-park-basics%22%2C%22fuzz%22%2C%22built-phet-io%22%5D%2C%22snapshotName%22%3A%22snapshot-1652655453957%22%2C%22timestamp%22%3A1652663069319%7D&fuzz&memoryLimit=1000&phetioStandalone:956:1745)
at u.getDerivedPropertyListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1652655453957/energy-skate-park-basics/build/phet-io/energy-skate-park-basics_all_phet-io.html?continuousTest=%7B%22test%22%3A%5B%22energy-skate-park-basics%22%2C%22fuzz%22%2C%22built-phet-io%22%5D%2C%22snapshotName%22%3A%22snapshot-1652655453957%22%2C%22timestamp%22%3A1652663069319%7D&fuzz&memoryLimit=1000&phetioStandalone:956:2465)
Which seem related to this issue. Probably introduced an issue, but there is a small chance it may have uncovered a pre-existing issue. I may need help on this part on Monday. The changes in this file would not be easy to revert--alternatively, devs blocked by this issue may prefer to comment out parts of the DerivedProperty.dispose
body, like this.dependencies = null;
.
@marlitas and I pushed a proposed fix
multilink<>
and DerivedProperty<>
(no type parameters)Multilink.multilink
and Multilink.lazyMultilink
. T,T1=unknown
Commits in the way to move things to Multilink.
Latest checklist in case it gets hidden by commits: https://github.com/phetsims/axon/issues/395#issuecomment-1132032206
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.
Fixed some aqua imports above
@samreid, I see a couple of changes that may have broken the PhET-iO API. For example https://github.com/phetsims/joist/commit/4128a846f6b16567861be0bd2adbd99f369e3a69. 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.
@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 https://github.com/phetsims/chipper/issues/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.