-
Notifications
You must be signed in to change notification settings - Fork 56
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
Fixes typescript generics to be correct for the returned proxy object #50
Conversation
? T[KK] | ||
: T[KK] extends (...args: infer A) => infer R | ||
? (...args: A) => Promise<R> | ||
: T[KK] |
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.
This is why we can't have nice things. 😆 I'm going to need to take some time to make sure I understand this so I can maintain it in the future.
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.
Yeah the syntax is kind of funky, they are mapped types
https://www.typescriptlang.org/docs/handbook/advanced-types.html#mapped-types
Honestly I copied most of it from the 3.x branch type defs and added the bit to restrict to only function properties. I should have changed the generic type names so they were a bit more meaningful than T[KK]
, but I was in a bit of a rush, my bad.
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.
Hey @marlon-tucker, I'm trying to get a better understanding of how these changes improve the experience of the developer using Penpal. Again, I'm new with TS. I thought it might help prevent me from doing something incorrect, like this, since I'm not accounting for a promise being returned from multiply
:
import { connectToChild } from "penpal";
const connection = connectToChild({
iframe: new HTMLIFrameElement()
});
type Child = {
multiply(...args: number[]): number
};
connection.promise.then((child: Child) => {
const result: number = child.multiply(1, 3);
});
But I think I'm missing something fundamental. Maybe you have an example of how you're leveraging these types? And maybe an example of problems that the changes in this PR prevent? It would be helpful to me.
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.
Sure no worries, taking the code from your example:
import { connectToChild } from "penpal";
// this could be exported from a shared code library, and
// the same interface the child codebase uses to make sure
// it's implementing the expected API.
interface ChildApi {
multiply(...args: number[]): number;
}
// supply as a generic type arg:
const connection = connectToChild<ChildApi>({
iframe: new HTMLIFrameElement()
});
// using async/await in place of promises for compactness but same end result.
const child = await connection.promise;
const result = await child.multiply(1, 3);
The benefit this gives is the child codebase can implement the shared API interface without having to return a promise object if it doesn't need to. However when penpal creates the connection, all the api methods are promise based, and this mechanism enforces that with type inference.
The downside is the discoverability of this feature as it remains hidden until you supply a type argument to connectToChild
.
Of course, the alternative way to tackle this would be to just create a penpal specific interface which contained methods which all return promises and manually type the return result from connection.promise
like you did in your example.
As always there's no one correct answer to this, it's more about project API design etc.,
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.
Oh I see. Yeah, that's pretty wild. Okay, I'll merge your changes. Thanks Marlon.
Thanks @marlon-tucker. |
Apologies for creating extra work for you but I screwed up in the previous pull request.
I neglected the fact that the returned promise is an proxy object which all methods return a promise object.
This fixes the typescript typings so the generic argument is automatically mapped to a type which the following: