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

Why not use object/class as output similar like Rust Result (Ok/Err class) instead of array? #22

Closed
2 tasks done
mharj opened this issue Aug 19, 2024 · 9 comments
Closed
2 tasks done

Comments

@mharj
Copy link

mharj commented Aug 19, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Versions

A minimal reproducible example

https://github.com/luolapeikko/result-option/blob/main/test/Result.test.ts

Description

Array is kind of problematic output as then you have two values to resolve again, so I would like to see something like Rust Result like setup.

const value = 'value';
const result ?= await testFunction(() => value);
expect(result.isOk).to.be.true;
expect(result.isErr).to.be.false;
expect(result.ok()).to.be.equal(value);
expect(result.err()).to.be.equal(undefined);
expect(result.unwrap()).to.be.equal(value);
const err = new Error('test');
const result ?= await testFunction(() => {
  throws err;
});
expect(result.isOk).to.be.false;
expect(result.isErr).to.be.true;
expect(result.ok()).to.be.equal(undefined);
expect(result.err()).to.be.eql(err);
expect(() => result.unwrap()).to.throw(err);

Steps to Reproduce

Expected Behavior

No response

@gabrielricardourbina
Copy link

This is the way to go, this is already solved in standard JS Promise.allSettled

@DScheglov
Copy link

@gabrielricardourbina

The first of all Rust's Result has both err and ok values typed, the Promise and PromiseSettledResult have only fulfilled value typed.

The second, Promise -- is a great stuff, but we shouldn't use it where we don't need it.

So, Promise.allSettled covers only small part of use cases

@arthurfiorette
Copy link
Owner

This proposal aims to increase the usage of try/catch expressions and error handling, not to create a helper Result class that you can create with current js code and publish it no npm.

Tuples were chosen because they are simple and makes total sense for that context. Promise.allSettled could never use tuples because the end result would look something like:

const [[error1, data1], [error2, data2]] = await Promise.allSettled(fn1(), fn2())

Which is far less readable than just what we are trying to achieve here:

const [error, data] = try fn() // new syntax chosen by community

@arthurfiorette arthurfiorette closed this as not planned Won't fix, can't repro, duplicate, stale Aug 21, 2024
@DScheglov
Copy link

DScheglov commented Aug 21, 2024

@arthurfiorette

With all due respect, how is it possible to refer to Rust in the Proposal Motivation and claim that Result is a helper type that could easily be published to npm?

I have published the one ... @cardellini/ts-result ... and what do I have?

import { Result, ok, err } from '@cardellini/ts-result';

type JsonObject = Record<string, unknown>;

const okIfObject = (value: unknown): Result<JsonObject, 'ERR_NOT_AN_OBJECT'> =>
  typeof value === 'object' && value !== null && !Array.isArray(value)
    ? ok(value as JsonObject)
    : err('ERR_NOT_AN_OBJECT');

const okIfInt = (value: unknown): Result<number, 'ERR_NOT_AN_INT'> =>
  Number.isInteger(value)
    ? ok(value as number)
    : err('ERR_NOT_AN_INT');

const okIfString = (value: unknown): Result<string, 'ERR_NOT_A_STRING'> =>
  typeof value === 'string'
    ? ok(value)
    : err('ERR_NOT_A_STRING');

type Person = {
  name: string;
  age: number;
}

const okIfPerson = (value: unknown): Result<Person, 'ERR_NOT_A_PERSON'> =>
  Do(function*() {
    const obj = yield* okIfObject(value);
    const name = yield* okIfString(obj.name);
    const age = yield* okIfInt(obj.age);

    return { name, age };
  }).mapErr(() => 'ERR_NOT_A_PERSON');

const person: Person = okIfPerson({ name: 'John', age: 42 }).unwrap();

I have to use generators to write monadic code in imperative style.
And it is currently the best option (EffectTS offers the same approach,
fp-ts ... -- offers terrible .bind chaining).

I'd like to write something like that:

const okIfPerson = (value: unknown) => {
  const obj = try? okIfObject(value);
  const name = try? okIfString(obj.name);
  const age = try? okIfInt(obj.age);

  return { name, age };
}

const person = try! okIfPerson({ name: 'John', age: 42 });

Result in Rust is not a helper class, it is exactly a way to do error handling.
Rust also has some analog of exceptions: panic.

So, while this Proposal pretends to Symbol.result -- it must consider the Result type
and typed errors, despite of the initial disclaimer.

In the same time this proposal could be easlity replaced with simple helper, that is indeed helper:
doTry:

import doTry from 'do-try-tuple';

function div(a: number, b: number): number {
  if (b !== 0) return a / b;
  if (a !== 0) throw new Error(`Division by Zero`);
  throw new Error('Indeterminate Form');
}

const [errX, x] = doTry(() => div(4, 2));

if (errX == null) {
  const doubleX = x * 2;
  console.log('doubleX:', doubleX);
}

Or something like dotry, that even supports iterators and async iterators:

import _try from "dotry";

async function* iterateAsync(...data: number[]) {
    for (let num of data) {
        if (num > 9) {
            throw new RangeError(`number ${num} is out of range`);
        } else {
            yield num;
        }
    }
}

for await (let [err, value] of _try(iterateAsync, 1, 2, 3, 4)) {
    // ...
}

// Or
for await (let [err, value] of _try(iterateAsync(1, 2, 3, 4))) {
    // ...
}

To write own implementation is also -- not a big deal ...
Why do we need this proposal???

@anacierdem
Copy link

Thank you @DScheglov. I am also 100% into creating a monadic error handling mechanism. Current proposal is just “resembling” that idea but in reality doesn’t go beyond being a dangerous syntactic sugar.

@DScheglov
Copy link

@anacierdem
actually the exception is a monadic approach, as well. They are just not typed ...

@arthurfiorette
Copy link
Owner

arthurfiorette commented Aug 22, 2024

@DScheglov i hope you can see the example you brought up is very verbose and confusing for first readers.

The above would require a npm package to be installed, so it will be out of at least 99% of tutorials out there, so we will still see patterns like JSON.parse being called without handling possibility of errors.

The package you created also has not web bundles, so anyone trying to use it in his simple index.html file would have to setup a bundler to be able to use your package (293 kB unpacked).

And so on...

#9 (comment)

@arthurfiorette
Copy link
Owner

Let's take you as an example, you talk so much about how a lib is so simple to use and that there is no reason to facilitate error handling in JS.

If we search for JSON.parse in your github account, there's 21 matches, although some of them are in tests, not a single one JSON.parse call is inside a trycatch for error handling, even packages available on npm.

Exactly, other peoples code could crash because you haven't handled the case where JSON.parse could fail.

Don't you think that if just by getting used to add try before some function calls you know may fail at runtime (JSON.parse(), fetch(), query()) and easily handle a possibility of error would drastically reduce the changes of your JS program to crash?

And i'm not bashing you in any sense, is just that the above happens to literally everyone.

@DScheglov
Copy link

DScheglov commented Aug 22, 2024

@arthurfiorette

i hope you can see the example you brought up is very verbose and confusing for first readers.

Could you please be more conrete here -- I've brought up too many samples

so we will still see patterns like JSON.parse being called without handling possibility of errors.

Perhaps, you will be surprised, but not each thrown exception must be caught directly on the function call.
More then in 90% (or even more) cases it must not be.

As instance, do you know that JSON.stringify also throws -- how often you wrap JSON.stringify in try .. catch?

The package you created also has not web bundles, so anyone trying to use it in his simple index.html file would have to setup a bundler to be able to use your package (293 kB unpacked).

  1. I guess if someone writes code directly in index.html, they can just do copy paste of correspondent function to avoid using a bundler
  2. I will add support for browsers
  3. As I said, to write the own version of this helper takes 5 minutes! Just 5 minutes.

you talk so much about how a lib is so simple to use and that there is no reason to facilitate error handling in JS.

Am I talking?? I'm talking that to write own is so simple! yes I'm!
nice-try ~ 12mln. downloads per week )))
Yes, perhaps it is simple to use

Don't you think that if just by getting used to add try before some function calls you know may fail at runtime (JSON.parse(), fetch(), query()) and easily handle a possibility of error would drastically reduce the changes of your JS program to crash?

There are three HUGE problems in this statement

  1. "some function" -- which one exactly, how to define??? As I mention why not before JSON.stringify?. What is the rule for that?
  2. "you know" -- how do you know? One of the largest problem with JS and with TS, as well -- We don't have a way to declare what exactly function throws and what it throws at all. We have @throws in jsdoc, but ... it is just a comment, that could be outdated or could be omit at all. Have you checked how often the @throws is used on github? Please check. And then we can continue talking about using try before function we know that may throw.
  3. "easily handle" -- indeed?? easily??? Do console.error(err) you mean this??? Easily!!! ))) To do what you have done in the motivation part of this proposal is easy:
async function getData() {
 const [requestError, response] ?= await fetch(
   "https://api.example.com/data"
 )

 if (requestError) {
   handleRequestError(requestError)
   return
 }
}

Yes, it is easy! To write handleRequestError(requestError) is easy! Return undefined is easy. Who cares that on the code which cals this getData() you have to check the nullable value? And not just check! You have to handle it in some way, otherwise this nullable value will be spread among entire the project - and it definitely will make you code LESS reliable.

To handle errors is not easy!

My approach to errors is the following:

  1. Define expected errors
  2. Express expected errors in the Domain terms (if it is impossible -- the error must not be expected)
  3. Declare the expected errors
  4. Ensure that error declaration is spread through the possible call stack
  5. Ensure that all possible expected errors are correctly reflected to the client (if we are writing API: ensure that all returning errors are declared in the response schema, if we are writing front-end, ensure that for all errors messages are correctly displayed)
  6. Build Error Boundary to handle all unexpected errors and allow functions to throw them
  7. On the Error Boundary:
    7.1. report errors to error tracking tool
    7.2. sanitize error before responding to client

It doesn't seems to be easy, but it is my rules to write function like handleRequestError. In 99% cases -- I don't write anything and allow fetch to throw and then I'm reporting the error to Sentry. There are somecase when I really need to handle:

  • when the url to fetch is an input by client
  • when I assume potential internet issues that must be handled in some specific way, but I wil do it in one single place of code, and not each time I call the fetch (and yes, I do inject the fetch, instead of use the global reference).

And i'm not bashing you in any sense, is just that the above happens to literally everyone.

In general I'm excited with the proposal, and everything I'm writing, I'm writing exactly to improve it.
But your last comments manifest some gaps in understanding the error handling practicies.
Have you ever though about why exceptions are propagated by default? Just a tradition???

Bashing me ... you welcome to do that -- It will help me to become better.

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

5 participants