Open MarcusCemes opened 1 year ago
As an extension to the above, most functions in the Option
module will cause runtime errors when null
is involved, which is often the case with Prisma. For example, the following code:
function getUserWithToken(token: string): Promise<O.Option<User>> {
return prisma.user.findUnique({ where: { token } }); // returns "Promise<User | null>"
}
is valid TypeScript. However, when operating on the Option
type (using O.map()
):
const maybeUser = await getUserWithToken("token")'
const maybeName = O.map(user => user.name);
causes a runtime error TypeError: Cannot read properties of null (reading 'name')
. Again, TypeScript assumes that null
is a valid type for None
, however, ts-belt makes the assumption that an Option can only be undefined
.
Example
The following code is deemed type-safe by TypeScript:
However, it will throw at runtime. The IIFE simulates a Prisma select query, which returns a nullable value.
Explanation
None
is defined as eithernull
orundefined
:https://github.com/mobily/ts-belt/blob/c825d9709de1e5d15b1b362429e0cee56c96c516/src/Option/index.ts#L3C1-L6C47
However, the generated code for the
isSome()
andisNone()
type guards only checks for theundefined
case:While this code is faster, it assumes that the
Option
was constructed using the library's constructors, such asO.fromNullable()
, which convertsnull
toundefined
:Unlike
Result
,Option
is just a union with primitive types. TypeScript will implicitly cast aUser | null
to anOption<User>
, which is an alias forUser | null | undefined
, allowing it to be used as the parameter of theisSome
andisNone
functions, causing the runtime error.Solution
It's possible to use
isNull()
orisObject()
in the guards module instead, however,isSome()
andisNone()
are still unsafe type predicates. They acceptnull
values but do not check them (for the reasons stated above). I understand that they shouldn't be used in those contexts, but they can be used, and I'm worried that a colleague will make the same mistake and cause unexpected runtime errors that are difficult to track down that could easily have been avoided by checking fornull
and a negligible performance impact...Personally, I would recommend removing
undefined
from theNone
type.null
has a semantic value meaning "nothing", whileundefined
signifies more than the parameter or variable has not been set. This does not have a performance impact, andundefined
can be removed fromSome
. I'm not familiar with Rescript, but unfortunately, it seems to me thatundefined
is hardcoded as the TS equivalent ofNone
.