Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: typedef of E() proxy parameter constraint #1224

Closed
wants to merge 2 commits into from
Closed

fix: typedef of E() proxy parameter constraint #1224

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
103635f
fix: typedef of E() proxy parameter constraint
turadg Jun 23, 2022
c4f23d7
WIP for matthieu to play with
turadg Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/eventual-send/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>>;
Copy link
Contributor

@mhofman mhofman Jun 23, 2022 •
edited
Loading

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

@turadg turadg Jun 23, 2022 •
edited
Loading

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:
Screen Shot 2022-06-23 at 2 10 53 PM

Basically it's saying, "if you're trying to use a method then the argument has to be defined".

Using never gives this feedback,
Screen Shot 2022-06-23 at 2 09 27 PM
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.

Copy link
Contributor

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 ?


/**
* E.get(x) returns a proxy on which you can get arbitrary properties.
Expand All @@ -245,7 +245,7 @@ interface EProxy {
* @param x target for property get
* @returns property get proxy
*/
readonly get: <T>(x: T) => EGetters<LocalRecord<T>>;
readonly get: <T extends {}>(x: T) => EGetters<LocalRecord<T>>;

/**
* E.resolve(x) converts x to a handled promise. It is
Expand Down
27 changes: 27 additions & 0 deletions packages/eventual-send/src/index.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,30 @@ const foo2 = async (a: FarRef<{ bar(): string; baz: number }>) => {
// @ts-expect-error - calling directly is valid but not yet in the typedef
a.bar;
};

// Nullish handling
type SomeRemotable = { someMethod: () => 'hello'; someVal: 'alsoHello' };
const undefinedCase = () => {
let ref: SomeRemotable | undefined;
// @ts-expect-error could be undefined
E(ref).someMethod();
// @ts-expect-error optional chaining doesn't work with E()
E(ref)?.someMethod();
// @ts-expect-error could be undefined
E.get(ref);
const getters = E.get(ref!);
getters.someMethod.then(sayHello => sayHello());
getters.someVal;
};
const nullCase = () => {
let ref: SomeRemotable | null;
// @ts-expect-error could be null
E(ref).someMethod();
// @ts-expect-error optional chaining doesn't work with E()
E(ref)?.someMethod();
// @ts-expect-error could be null
E.get(ref);
const getters = E.get(ref!);
getters.someMethod.then(sayHello => sayHello());
getters.someVal;
};