Use Result type from @polywrap/result instead of InvokeResult

[nerfZael]

The InvokeResult type returned from invocations is defined as follows:

/**
 * Result of an Wrapper invocation.
 *
 * @template TData Type of the invoke result data.
 */
export interface InvokeResult<TData = unknown> {
    /**
     * Invoke result data. The type of this value is the return type
     * of the method. If undefined, it means something went wrong.
     * Errors should be populated with information as to what happened.
     * Null is used to represent an intentionally null result.
     */
    data?: TData;
    /** Errors encountered during the invocation. */
    error?: Error;
}

It should be replaced with the Result type from @polywrap/result to get better type checking from TypeScript

export type Result<T, E = undefined> =
  | { ok: true; value: T }
  | { ok: false; error: E | undefined };

[krisbitney]

Related: #1018

[krisbitney]

Should the error in the Result type be an Error? or extend Error?

Also, is Result really different from InvokeResult? It seems like InvokeResult provides the same functionality as Result, but in a more TypeScript-idiomatic way.

[dOrgJelli]

Yah I'm a bit conflicted here... it is more "expensive" to pass around a Result object because it has a boolean in addition to the property. What are the cases where Result functions better than InvokeResult?

[krisbitney]

I've been going through the core and replacing thrown exceptions with Result. I love the UX of Result and now I think it is a really positive change!

I do still think the error type should be bounded to E extends Error. Without the bound, we get situations like client.tryResolveUri returning Promise<Result<UriPackageOrWrapper, unknown>>. Users should know what type to expect. I think even allowing undefined is probably wrong, because we can always say something about the cause of an error.

export type Result<T, E extends Error> =
  | { ok: true; value: T }
  | { ok: false; error: E };

What do you think? You're really talented and much more experienced than me, so I'm guessing you already thought about this a lot. Maybe you've already had this conversation lol.

Also, is it okay if I go through the URI Resolvers and change the unknown error type to Error?

[nerfZael]

It should not extend Error. The error from result type can be anything (just a string, or an enum of errors, or a union of error strings etc.) depending on the needs of the user. Error (js Errors) are meant to be thrown, the error from the result type is not necessarily thrown (in fact, it enables recovering from errors).

Actually, the Result type is more idiomatic which I think you've come to see from your comment below :)

[nerfZael]

Yah I'm a bit conflicted here... it is more "expensive" to pass around a Result object because it has a boolean in addition to the property. What are the cases where Result functions better than InvokeResult?

The boolean being expensive is a non-issue IMO.
Additionally, it covers cases where the function that returns Result can fail but has no defined error.

Here is an example image of some differences in practice:

For InvokeResult, even though test1 in the event of success can only return a non-nullable string, the actual data type of data is a nullable one. This is not the case for Result.
Additionally, even if you check for an error with InvokeResult, that error still appears on the InvokeResult type. Unlike with Result where it's not defined if ok is true.

Also, InvokeResult does not cover the cases where the data is void.

[nerfZael]

I've been going through the core and replacing thrown exceptions with Result. I love the UX of Result and now I think it is a really positive change!

I do still think the error type should be bounded to E extends Error. Without the bound, we get situations like client.tryResolveUri returning Promise<Result<UriPackageOrWrapper, unknown>>. Users should know what type to expect. I think even allowing undefined is probably wrong, because we can always say something about the cause of an error.

export type Result<T, E extends Error> =
  | { ok: true; value: T }
  | { ok: false; error: E };

What do you think? You're really talented and much more experienced than me, so I'm guessing you already thought about this a lot. Maybe you've already had this conversation lol.

Also, is it okay if I go through the URI Resolvers and change the unknown error type to Error?

We already had a call on this topic, but I'll post the notes here to document them:
I already explained above why Error should not be extended, but here's an example use:

enum InvokeErrorType {
  UriNotFound = 1,
  SubInvokeError = 2,
  SomeOtherError 3,
}
const result = client.invoke<number, InvokeErrorType>();

Or with union strings:

type InvokeErrorType = "UriNotFound" | "SubInvokeError" | "SomeOtherError"; 
const result = client.invoke<number, InvokeErrorType>();

Now that the errors are all accounted for, the user can safely and adequately recover from them. (E.g. with a switch(result.error) statement).

As for the unknown and how and where it should be used (Like it's used in uri resolvers):
unknown is used where the error is not important or can not be known and is just being passed around by middleware.
E.g. the IUriResolver<T> interface is usually referent to IUriResolver<unknown> because the places where it's used there is no way of determining type and it is not important since in those places the error is not being handled.
Replacing unknown with Error would serve no purpose because you would still not know the actual type, nor would you use it.

That all said, client.invoke could return the type Promise<Result<TData, Error>> to make it compatible with the current way users expect the code to work and since we are catching thrown errors in the invoke function.

[krisbitney]

I hear you, but after thinking about it for a couple of days I'm still unconvinced. You don't need to convince me, of course, but I want to make a case for the alternative. And of course I want to caveat everything I say with some humility: maybe I just don't understand it!

First, I want to say I'm not opposed to Result<T, E> being an unbounded generic so much as I'm uneasy about the way it's being used.

I think this will cause unnecessary headaches: export interface IUriResolver<TError = undefined>.

And I don't think this is feasible: client.invoke<TData, TError>(...)

It seems the generics approach would require users to know which type of error to expect. One URI resolver could be returning an enum, another a string, another an Error? And that's just URI resolvers--i.e. it's not considering the errors that can occur elsewhere in the client, core, wasm-js, or in the wrappers themselves. It's too much. Most devs want to focusing on building their projects. UX and productivity matter!

In Rust, users typically know what type of Err they will receive. Library developers specify the Err type a Result will contain. For example, a function might return Result<Foo, String>. Users don't have to tell a method what type of Err it might return. I'm still fairly new to Rust, but this has been my experience.

Errors are meant to be thrown, but not necessarily immediately. And Errors are also meant to be handled (sometimes by ignoring them!). The difference between returning a Result and throwing an Error is that the Result is more explicit--i.e. it tells the user that something could have gone wrong and lets the user handle it however they wish. Maybe we aren't throwing the Error, but the app developer may choose to.

Error types are useful because they are easy and informative. It should be easy for users to learn what went wrong and debug. I think it's better to be "opinionated" about this sort of thing.

E.g. the IUriResolver interface is usually referent to IUriResolver because the places where it's used there is no way of determining type and it is not important since in those places the error is not being handled.
Replacing unknown with Error would serve no purpose because you would still not know the actual type, nor would you use it.

I think it serves an important purpose! If you don't think it matters, do you mind if I change it to IUriResolver<TError extends Error | undefined = undefined> and update the URI resolvers accordingly?

[nerfZael]

First, I want to say I'm not opposed to Result<T, E> being an unbounded generic so much as I'm uneasy about the way it's being used.

Yea, I can agree with that to an extent.

And I don't think this is feasible: client.invoke<TData, TError>(...)
It should be: And I don't think this is feasible: client.invoke(...)
If you check my comment above: "That all said, client.invoke could return the type Promise<Result<TData, Error>> to make it compatible with the current way users expect the code to work and since we are catching thrown errors in the invoke function."

It seems the generics approach would require users to know which type of error to expect.
Exactly! Otherwise all they get (if you return them an Error) is a message string. It should be expected of users to know what they have configured the resolver (and implicitly the invoke method) to return. They are the ones supplying the resolver, after all.

I think where most of the confusion comes from is that the current client doesn't manage errors all that well, and so the errors can be anything regardless of what the user configures. What I did with the unbounded generic type for error in the resolvers is moved the responsibility of the client's configuration to the user. It's only a small step in that direction.

If every error extends Error | undefined, then you can't have the following:

export type CustomError = 
  | "..."
  | "...";
export class CustomResolver implements IUriResolver<CustomError> {
  tryResolveUri(
    uri: Uri
  ): Promise<Result<UriPackageOrWrapper, CustomError>> {
    ...
  }
}

This is an example of where the user can account for all the errors and uses a string union to define them (instead of the much heavier Error type).

Of course, this is not possible with the invoke part of the client since there can be a lot of different error thrown.
Ideally, the client would actually be a generic over the resolution error type:

export interface ClientConfig<TUri extends Uri | string = string, TResolutionError> {
  redirects: UriRedirect<TUri>[];
  interfaces: InterfaceImplementations<TUri>[];
  envs: Env<TUri>[];
  resolver: IUriResolver<TResolutionError>;
}

If we defined the Client like this, then users could still use any error type (like in the above example) if they want to implement their own version of the Client, but in the PolywrapClient we can implement the following as a short term solution:

export class PolywrapClient implements Client<string, Error> {
...
}