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.

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

Eliminate Need for both Union Type and Method Interface #2

Open
alexlafroscia opened this issue Mar 11, 2022 · 0 comments
Open

Eliminate Need for both Union Type and Method Interface #2

alexlafroscia opened this issue Mar 11, 2022 · 0 comments
Assignees

Comments

@alexlafroscia
Copy link
Owner

alexlafroscia commented Mar 11, 2022

Right now, Result<T, E> is defined as a union between Ok<T> and Err<E>. This is so that properties like isOk and isErr can narrow a possible Result<T, E> into either an Ok<T> or an Err<E>, which can be useful, especially in cases where you have a situation like this:

function doSomething(input: Result<T, E>): Result<U, E> {
  if (input.isErr) {
    // No errors because we *know* that `input` is `Err<E>` which fits `Result<U, E>`
    return input;
  }

  // Otherwise do stuff
}

The problem is that there is nothing that ensures that Ok<T> and Err<E> have the same methods and there is no great place to document the methods from the perspective of calling them on a Result<T, E>; we can document them on Ok<T> and on Err<E> but that feels awkward, as the actual end-user is not usually working with one of those types. So, a second representation of Result is defined (ResultMethods) that is an interface that both of them implement. This helps with ensuring the methods are the same on both classes, and helps with type information in a user's editor, but is kind of awkward from a documentation standpoint; I'm having a real hard time getting the Deno docs page to link Result to ResultMethods so that people can actually navigate around the pages to find the method documentation.

Initially, this library was written differently; Result was an abstract base class that Ok and Err implemented, which felt a little nicer. However, this approach makes us lose the ability to use isOk and isErr to narrow the type from Result to Ok or Err:

Getters cannot narrow the type of the object they are called on
microsoft/TypeScript#43368

Methods have the same problem
microsoft/TypeScript#44212

So for now, we're left with both a Result<T, E> type and the ResultMethods interface.

If either of the above TypeScript issues get resolved, it would be a really nice ergonomic "win" for this library; it wouldn't change the way the library is used, but it would make the code easier to write and documentation easier to read.

alexlafroscia added a commit that referenced this issue Mar 21, 2022
This removes the union type and interface for each type and uses just an interface.

The `is___` methods no longer can discriminate between types fully; it only ensures that the instance is a single particular type. This simplifies the architecture of the library and makes documentation easier at the expense of not being able to be as exhaustive about the types in conditionals. For example, an `if/else` condition that checks for `isSome` no longer knows that the `else` condition contains a `None`.

Related to #2
@alexlafroscia alexlafroscia self-assigned this Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant