Skip to content

Commit

Permalink
Stop reusing the val property in both Ok and Err
Browse files Browse the repository at this point in the history 
This is a very backwards incompatible change.

Basically the problem is when we had Result<T, E> and T was actually the
same as E (or compatible enough) the following mixup was possible where
the TS compiler wouldn't say anything[1]:

    const userIdOkTest: Result<string, string> = Ok("00287b3a-12fb-455c-b3d9-e662153c91b6");
    const userIdErrTest: Result<string, string> = Err("User not found");

    await doSomethingWithUser(userIdErrTest.val); // runtime error because the compiler didn't complain.

    // More examples that are not checked for ok or err
    if (userIdOkTest.val === "test") {
        console.log("ok or err not checked but it compiles.")
    } else if (userIdErrTest.val === "test") {
        console.log("ok or err not checked but it compiles.")
    }

Eliminating the overlap between Ok and Err and providing separate
properties for each resolves the issue.

[1] vultix#69
  • Loading branch information
@jstasiak
jstasiak committed Aug 4, 2023
1 parent 04a484c commit 177e189
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 67 deletions.
28 changes: 14 additions & 14 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,10 @@ function readFile(path: string): Result<string, 'invalid path'> {
const result = readFile('test.txt');
if (result.ok) {
// text contains the file's content
const text = result.val;
const text = result.value;
} else {
// err equals 'invalid path'
const err = result.val;
const err = result.error;
}
```

Expand Down Expand Up @@ -178,19 +178,19 @@ _Note: Typescript currently has a [bug](https://github.com/microsoft/TypeScript/
```typescript
let result: Result<number, Error> = Ok(1);
if (result.ok) {
// Typescript knows that result.val is a number because result.ok was true
let number = result.val + 1;
// Typescript knows that result.value is a number because result.ok was true
let number = result.value + 1;
} else {
// Typescript knows that result.val is an `Error` because result.ok was false
console.error(result.val.message);
// Typescript knows that result.error is an `Error` because result.ok was false
console.error(result.error.message);
}

if (result.err) {
// Typescript knows that result.val is an `Error` because result.err was true
console.error(result.val.message);
// Typescript knows that result.error is an `Error` because result.err was true
console.error(result.error.message);
} else {
// Typescript knows that result.val is a number because result.err was false
let number = result.val + 1;
// Typescript knows that result.value is a number because result.err was false
let number = result.value + 1;
}
```

Expand Down Expand Up @@ -396,9 +396,9 @@ const greaterThanZero = obs$.pipe(

greaterThanZero.subscribe((result) => {
if (result.ok) {
console.log('Was greater than zero: ' + result.val);
console.log('Was greater than zero: ' + result.value);
} else {
console.log('Got Error Message: ' + result.val);
console.log('Got Error Message: ' + result.error);
}
});

Expand Down Expand Up @@ -502,9 +502,9 @@ const test$ = obs$.pipe(

test$.subscribe((result) => {
if (result.ok) {
console.log('Got string: ' + result.val);
console.log('Got string: ' + result.value);
} else {
console.log('Got error: ' + result.val.message);
console.log('Got error: ' + result.error.message);
}
});

Expand Down
54 changes: 27 additions & 27 deletions src/result.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export class ErrImpl<E> implements BaseResult<never, E> {

readonly ok!: false;
readonly err!: true;
readonly val!: E;
readonly error!: E;

private readonly _stack!: string;

Expand All @@ -181,7 +181,7 @@ export class ErrImpl<E> implements BaseResult<never, E> {

this.ok = false;
this.err = true;
this.val = val;
this.error = val;

const stackLines = new Error().stack!.split('\n').slice(2);
if (stackLines && stackLines.length > 0 && stackLines[0].includes('ErrImpl')) {
Expand All @@ -207,22 +207,22 @@ export class ErrImpl<E> implements BaseResult<never, E> {
// The cause casting required because of the current TS definition being overly restrictive
// (the definition says it has to be an Error while it can be anything).
// See https://github.com/microsoft/TypeScript/issues/45167
throw new Error(`${msg} - Error: ${toString(this.val)}\n${this._stack}`, { cause: this.val as any });
throw new Error(`${msg} - Error: ${toString(this.error)}\n${this._stack}`, { cause: this.error as any });
}

expectErr(_msg: string): E {
return this.val
return this.error
}

unwrap(): never {
// The cause casting required because of the current TS definition being overly restrictive
// (the definition says it has to be an Error while it can be anything).
// See https://github.com/microsoft/TypeScript/issues/45167
throw new Error(`Tried to unwrap Error: ${toString(this.val)}\n${this._stack}`, { cause: this.val as any });
throw new Error(`Tried to unwrap Error: ${toString(this.error)}\n${this._stack}`, { cause: this.error as any });
}

unwrapErr(): E {
return this.val;
return this.error;
}

map(_mapper: unknown): Err<E> {
Expand All @@ -234,31 +234,31 @@ export class ErrImpl<E> implements BaseResult<never, E> {
}

mapErr<E2>(mapper: (err: E) => E2): Err<E2> {
return new Err(mapper(this.val));
return new Err(mapper(this.error));
}

mapOr<U>(default_: U, _mapper: unknown): U {
return default_;
}

mapOrElse<U>(default_: (error: E) => U, _mapper: unknown): U {
return default_(this.val);
return default_(this.error);
}

or<T, E2>(other: Result<T, E2>): Result<T, E2> {
return other;
}

orElse<T, E2>(other: (error: E) => Result<T, E2>): Result<T, E2> {
return other(this.val);
return other(this.error);
}

toOption(): Option<never> {
return None;
}

toString(): string {
return `Err(${toString(this.val)})`;
return `Err(${toString(this.error)})`;
}

get stack(): string | undefined {
Expand All @@ -278,13 +278,13 @@ export class OkImpl<T> implements BaseResult<T, never> {

readonly ok!: true;
readonly err!: false;
readonly val!: T;
readonly value!: T;

/**
* Helper function if you know you have an Ok<T> and T is iterable
*/
[Symbol.iterator](): Iterator<T extends Iterable<infer U> ? U : never> {
const obj = Object(this.val) as Iterable<any>;
const obj = Object(this.value) as Iterable<any>;

return Symbol.iterator in obj
? obj[Symbol.iterator]()
Expand All @@ -302,61 +302,61 @@ export class OkImpl<T> implements BaseResult<T, never> {

this.ok = true;
this.err = false;
this.val = val;
this.value = val;
}

/**
* @see unwrapOr
* @deprecated in favor of unwrapOr
*/
else(_val: unknown): T {
return this.val;
return this.value;
}

unwrapOr(_val: unknown): T {
return this.val;
return this.value;
}

expect(_msg: string): T {
return this.val;
return this.value;
}

expectErr(msg: string): never {
throw new Error(msg);
}

unwrap(): T {
return this.val;
return this.value;
}

unwrapErr(): never {
// The cause casting required because of the current TS definition being overly restrictive
// (the definition says it has to be an Error while it can be anything).
// See https://github.com/microsoft/TypeScript/issues/45167
throw new Error(`Tried to unwrap Ok: ${toString(this.val)}`, { cause: this.val as any });
throw new Error(`Tried to unwrap Ok: ${toString(this.value)}`, { cause: this.value as any });
}

map<T2>(mapper: (val: T) => T2): Ok<T2> {
return new Ok(mapper(this.val));
return new Ok(mapper(this.value));
}

andThen<T2>(mapper: (val: T) => Ok<T2>): Ok<T2>;
andThen<E2>(mapper: (val: T) => Err<E2>): Result<T, E2>;
andThen<T2, E2>(mapper: (val: T) => Result<T2, E2>): Result<T2, E2>;
andThen<T2, E2>(mapper: (val: T) => Result<T2, E2>): Result<T2, E2> {
return mapper(this.val);
return mapper(this.value);
}

mapErr(_mapper: unknown): Ok<T> {
return this;
}

mapOr<U>(_default_: U, mapper: (val: T) => U): U {
return mapper(this.val);
return mapper(this.value);
}

mapOrElse<U>(_default_: (_error: never) => U, mapper: (val: T) => U): U {
return mapper(this.val);
return mapper(this.value);
}

or<E2>(_other: Result<T, E2>): Result<T, E2> {
Expand All @@ -368,7 +368,7 @@ export class OkImpl<T> implements BaseResult<T, never> {
}

toOption(): Option<T> {
return Some(this.val);
return Some(this.value);
}

/**
Expand All @@ -381,11 +381,11 @@ export class OkImpl<T> implements BaseResult<T, never> {
* (this is the `into_ok()` in rust)
*/
safeUnwrap(): T {
return this.val;
return this.value;
}

toString(): string {
return `Ok(${toString(this.val)})`;
return `Ok(${toString(this.value)})`;
}
}

Expand Down Expand Up @@ -416,7 +416,7 @@ export namespace Result {
const okResult = [];
for (let result of results) {
if (result.ok) {
okResult.push(result.val);
okResult.push(result.value);
} else {
return result as Err<ResultErrTypes<T>[number]>;
}
Expand All @@ -439,7 +439,7 @@ export namespace Result {
if (result.ok) {
return result as Ok<ResultOkTypes<T>[number]>;
} else {
errResult.push(result.val);
errResult.push(result.error);
}
}

Expand Down
18 changes: 9 additions & 9 deletions src/rxjs-operators/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export function elseMap<T, E, E2>(mapper: (val: E) => E2): OperatorFunction<Resu
return source.pipe(
map((result) => {
if (result.err) {
return mapper(result.val);
return mapper(result.error);
} else {
return result.val;
return result.value;
}
}),
);
Expand All @@ -47,7 +47,7 @@ export function elseMapTo<T, E, E2>(value: E2): OperatorFunction<Result<T, E>, T
if (result.err) {
return value;
} else {
return result.val;
return result.value;
}
}),
);
Expand All @@ -67,7 +67,7 @@ export function resultSwitchMap<T, E, T2, E2>(
return source.pipe(
switchMap((result) => {
if (result.ok) {
return mapper(result.val);
return mapper(result.value);
} else {
return of(result);
}
Expand Down Expand Up @@ -96,7 +96,7 @@ export function resultMergeMap<T, E, T2, E2>(
return source.pipe(
mergeMap((result) => {
if (result.ok) {
return mapper(result.val);
return mapper(result.value);
} else {
return of(result);
}
Expand All @@ -116,7 +116,7 @@ export function filterResultOk<T, E>(): OperatorFunction<Result<T, E>, T> {
return (source) => {
return source.pipe(
filter((result): result is Ok<T> => result.ok),
map((result) => result.val),
map((result) => result.value),
);
};
}
Expand All @@ -125,7 +125,7 @@ export function filterResultErr<T, E>(): OperatorFunction<Result<T, E>, E> {
return (source) => {
return source.pipe(
filter((result): result is Err<E> => result.err),
map((result) => result.val),
map((result) => result.error),
);
};
}
Expand All @@ -135,7 +135,7 @@ export function tapResultErr<T, E>(tapFn: (err: E) => void): MonoTypeOperatorFun
return source.pipe(
tap((r) => {
if (!r.ok) {
tapFn(r.val);
tapFn(r.error);
}
}),
);
Expand All @@ -147,7 +147,7 @@ export function tapResultOk<T, E>(tapFn: (val: T) => void): MonoTypeOperatorFunc
return source.pipe(
tap((r) => {
if (r.ok) {
tapFn(r.val);
tapFn(r.value);
}
}),
);
Expand Down
6 changes: 3 additions & 3 deletions test/err.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ test('ok, err, and val', () => {
expect(err.ok).toBe(false);
assert<typeof err.ok>(false);

expect(err.val).toBe(32);
eq<typeof err.val, number>(true);
expect(err.error).toBe(32);
eq<typeof err.error, number>(true);
});

test('static EMPTY', () => {
expect(Err.EMPTY).toBeInstanceOf(Err);
expect(Err.EMPTY.val).toBe(undefined);
expect(Err.EMPTY.error).toBe(undefined);
eq<typeof Err.EMPTY, Err<void>>(true);
});

Expand Down
6 changes: 3 additions & 3 deletions test/ok.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ test('ok, err, and val', () => {
expect(err.ok).toBe(true);
assert<typeof err.ok>(true);

expect(err.val).toBe(32);
eq<typeof err.val, number>(true);
expect(err.value).toBe(32);
eq<typeof err.value, number>(true);
});

test('static EMPTY', () => {
expect(Ok.EMPTY).toBeInstanceOf(Ok);
expect(Ok.EMPTY.val).toBe(undefined);
expect(Ok.EMPTY.value).toBe(undefined);
eq<typeof Ok.EMPTY, Ok<void>>(true);
});

Expand Down
Loading

0 comments on commit 177e189

Please sign in to comment.