fix: typedef default onfulfilled handler for E.when #1233
Conversation
The return type of a reaction is generally not the same as the input type, which is why |
| E.when(Promise.all([aPromise, onePromise, remoteString])).then( | ||
| ([str, num, remote]) => { |
There was a problem hiding this comment.
The problem here is likely in a misusage of E.when, which is expected to be used like the following:
| E.when(Promise.all([aPromise, onePromise, remoteString])).then( | |
| ([str, num, remote]) => { | |
| E.when(Promise.all([aPromise, onePromise, remoteString]), ([str, num, remote]) => { |
There was a problem hiding this comment.
But the onfulfilled is optional, yes? This is where I was coming from https://github.com/Agoric/agoric-sdk/blob/21eb21d43390ba51c4d2bc5baecb1b684ee66c52/packages/run-protocol/src/vpool-xyk-amm/addPool.js#L70-L76
There was a problem hiding this comment.
Right, but I think it's optional in the sense that the expectation is to use either onfulfilled or onrejected. Using .then() with E.when() is somewhat of an anti-pattern.
That said, to enable this case with the types, I do believe that defaulting U to T will work.
There was a problem hiding this comment.
"expectation" and "anti-pattern" are fuzzy to me wrt the type defs. The type checker has only errors not warnings :)
If you think the type should require at least one handler, I can define the type that way. Though I think you're saying that it should allow them both to be omitted and your solution sounds right. I'll try it.
There was a problem hiding this comment.
Speaking only for then, the callback and errback default to exactly value => value and error => { throw error } respectively. They can both be omitted. when should be similar.
| @@ -257,11 +257,11 @@ interface EProxy { | |||
| * E.when(x, res, rej) is equivalent to | |||
| * HandledPromise.resolve(x).then(res, rej) | |||
| */ | |||
| readonly when: <T, U>( | |||
| readonly when: <T>( | |||
There was a problem hiding this comment.
I think by defaulting U to T (for when neither reactions are used) might solve the problem
| readonly when: <T>( | |
| readonly when: <T, U = T>( |
turadg
changed the title
| @@ -46,3 +46,15 @@ const foo2 = async (a: FarRef<{ bar(): string; baz: number }>) => { | |||
| // @ts-expect-error - calling directly is valid but not yet in the typedef | |||
| a.bar; | |||
| }; | |||
|
|
|||
| // when | |||
There was a problem hiding this comment.
If we're adding when type tests, do you mind also adding a test for the "normal" use case with a onfulfilled callback?
E.whenreturn type assumed anonfulfilledhandler is provided. That handler is optional and in https://github.com/Agoric/agoric-sdk/blob/21eb21d43390ba51c4d2bc5baecb1b684ee66c52/packages/run-protocol/src/vpool-xyk-amm/addPool.js#L70-L76 it wasn't used, which caused an error in the return type.This makes the return type default when a handler isn't provided.