-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: typedef of E() proxy parameter constraint #1224
Conversation
@@ -234,7 +234,7 @@ interface EProxy { | |||
* @param x target for method/function call | |||
* @returns method/function call proxy | |||
*/ | |||
<T>(x: T): ECallableOrMethods<RemoteFunctions<T>>; | |||
<T extends {}>(x: T): ECallableOrMethods<RemoteFunctions<T>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not update ECallableOrMethods
if extends null or undefined and return never
(or possibly a Record<PropertyKey, () => Promise<never>>
)? That would model more closely what is actually going on, where it's the action of using the result of E()
that is invalid, not using E itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it and see whether the feedback is better. I take it the change wouldn't affect what actually passes typecheck.
Do you have the same feedback for the get()
change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the only difference would be that const p = E(Promise.resolve(null))
is entirely valid, but that p.foo()
is the operation that is type invalid (However it is runtime valid, just results in a rejected promise (hence the Record<PropertyKey, () => Promise<never>>
suggestion).
I think E.get()
should be similar yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The never
is perhaps more accurate, but I wonder if constraining the input is more useful. Here's the error message with the PR version:
Basically it's saying, "if you're trying to use a method then the argument has to be defined".
Using never
gives this feedback,
which doesn't give the user much clue why or how to remedy it.
The never
is more sound than the generic constraint in that E(undefined)
would error in TS and not in runtime (is that true?). Though are their valid cases for E(undefined)
or even E(Promise<undefined>)
? If those aren't runtime errors now I wonder if they should be. Though I guess that gets back into the nature of the proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think where it matters most if for E(Promise<SomeRemotable | undefined>)
. For anything where the target is awaited first, the author should do a local check first. Maybe we could have the best of both worlds by doing extends {}
and have the second check in ECallableOrMethods
?
I think we're converging by doing both :) Each of our approaches has advantages and drawbacks, and I think doing both would get us the best of both while limiting the drawbacks. |
I thought I could solve a small pain point but it turned into more work than I had timeboxed so I'm closing it. |
The following throws
Cannot deliver "getChildNode" to target; typeof target is "undefined"
.It's tempting to make
E(undefined)
to returnundefined
but the behavior must be consistent with a promised value andPromise<undefined>
can't be determined before the object returns.So this solves the case above by updating the type definition to report that it won't work.