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

As-is, it is impossible to accurately refine types in a generic context #17

Closed
rbalicki2 opened this issue Aug 17, 2024 · 6 comments
Closed

Comments

@rbalicki2
Copy link

This makes it impossible to refine the type in a generic function

  • Consider this typescript playground. Here, we compare a tuple and a discriminated union and a generic function that can take a result. Since both T and E can contain null, it is impossible to determine whether the result is in the "error" or "ok" case. There are no such problems with a discriminated union.
    • Just as a note, I just realized that I wrote the tuple as [T, null] | [null, E], which is backwards. I won't fix that, because that's indicative of the fact that ordering is a footgun here, which is solved by using a discriminated union.
  • This means that these results are incompatible with every single API that doesn't further constraint at least one of T and E, which is to say, pretty much every single API.
  • One can maybe workaround this with a kludgy fix where every Result is declared as Result<T, Exclude<E, null>>, but I couldn't get it to work. But let's assume that something like this does work! That means that this constraint will propagate itself everywhere.

One response: you should never throw null.

  • I'll admit, it's not often that you need this.
  • This doesn't solve the types issue.
  • Is throw null a common pattern? Not directly. 6.1k results on GitHub. This is likely to be much more common than this indicates, however, due to code that accidentally throws null, such as throw lookUpUserReadableErrorMessage(e).
  • Is throw null a bad pattern? Not necessarily. If your error case contains no information, it only hurts performance to allocate objects, though one could conceivably throw true or something.

But a discriminated union also allocates an object

  • So does returning a tuple.

Conclusion

  • All of the problems listed as solved by using a discriminated union.
@gabrielricardourbina
Copy link

This is already solved in Promise.allSettled

@arthurfiorette
Copy link
Owner

Is throw null a common pattern? Not directly. 6.1k results on GitHub.

That's less than 0.1% and almost all 6.1k results comes from big libraries like nextjs, which uses throw null for different reasons.

@arthurfiorette
Copy link
Owner

[T, null] | [null, E] is the wrong type. As you can see with tuple-it. The correct type should be [E] | [null, T]

@arthurfiorette
Copy link
Owner

Related to #30

@DScheglov
Copy link

DScheglov commented Aug 21, 2024

@arthurfiorette

The type [E] | [null, T] is also incorrect: TS Playground, the type parameter E makes the discriminating this type impossible.

The correct type is [{}] | [null, T]: TS Playground

@rbalicki2
Copy link
Author

  • Is the safe way to check whether we are in an error state .length === 1? If so, that should be indicated in the docs. And that also means that we cannot destructure on the same line. (Maybe one can destructure as { '0': Error, '1': Value, 'length': isError }, though)
  • If we are requiring that E extends {} (aka is not null), that type will infect every other library. Consider https://github.com/Effect-TS/effect/blob/main/packages/effect/src/Effect.ts#L95, E does not extend {}. In order to be compatible with this new syntax, effect will have to enforce that E extends {} throughout.

Anyway, @DScheglov is right about the inability to discriminate in a generic context with that type.

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

4 participants