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

Tuples/Objects and Falsy errors. #30

Open
arthurfiorette opened this issue Aug 21, 2024 · 56 comments
Open

Tuples/Objects and Falsy errors. #30

arthurfiorette opened this issue Aug 21, 2024 · 56 comments
Assignees
Labels
enhancement New feature or request

Comments

@arthurfiorette
Copy link
Owner

arthurfiorette commented Aug 21, 2024

Additional post on twitter

As you might've seen in some issues around here. There's a discussion about the usage of [tuples] or {objects} as the result for this operator, as well as falsy errors, like throw undefined or more common cases like throw functionThatMightReturnUndefinedOrAnError().


Here's things that this proposal will NOT solve for you.

  • This proposal does not aims to create a Result<T> like object class with unary operations like .unwrap(), .mapError(fn) and/or .orElse(data). This can be easily achievable by current JS code and I'm pretty sure there's tons of libraries like this one out there.

  • This proposal will not change the behavior of any function or its return type. The safe assignment operator (or future try expression as of Discussion: Preferred operator/keyword for safe assignment #4) is to improve the caller experience and not the callee.


Return type syntaxes

I'll be using the chosen syntax at #4 for these examples.

Tuple:

Tuples are easier to destructure and follows the concept of error first destructuring.

Simple use case:

const [error, data] = try fn();

if (error) {
  console.error(error);
  return;
}

console.log(data);

Complex use case:

const [reqErr, response] = try await fetch('url')

if (reqErr) {
  console.error(reqErr);
  return;
}

const [parseErr, json] = try await response.json();

if (parseErr) {
  console.error(parseErr);
  return;
}

const [validateErr, validatedData] = try validate(json);

if (validateErr) {
  console.error(validateErr);
  return;
}

return validatedData;

Objects:

Simple use case:

const { error, data } = try fn();

if (error) {
  console.error(error);
  return;
}

console.log(data);

Complex use case:

const { error: reqErr, data: response } = try await fetch('url')

if (reqErr) {
  console.error(reqErr);
  return;
}

const maybeJson = try await response.json();

if (maybeJson.error) {
  console.error(maybeJson.error);
  return;
}

const { data: validatedData, error: validatedError } = try validate(maybeJson.data);

if (validatedError ) {
  console.error(validatedError );
  return;
}

return validatedData;

Both of them

An Object with error, data property and Symbol.iterator is also a valid solution.

Later we would need to improve the TS typings for that Symbol.iterator result to not mix error | data types on both destructure values, but since TS already did that for tuples ([1, 2, 3]) it won't be a problem.

Falsy values

Ideally, we should have some sort of wrapper against falsy errors, as in the end having falsy values wrapped are much helpful in such problems.

Besides intended use cases, no one expects to face a null inside a catch(error) block or neither will know how to handle that correctly.

Since errors are now treated as values, in both return types, we should have a way to differentiate them.

We have two main approaches:

helper property

Just like on Promise.allSettled's status property, we could have a .ok or something to tell the user that if the operation failed or not.

// obviously ok, could have as syntax like `fail` or a `status === 'err' || 'ok'`
const [ok, fnError, fnData] = try fn()
const { ok, error: fnError, data: fnData } = try fn()

if (!ok) {
  console.log(fnError)
}

const maybeResult = try fn()

if (!maybeResult.ok) {
  console.log(maybeResult.error)
}

Error wrapper

Wrapping the falsy value (0, '', null, ...) into a NullableError class or something could arguably be presented as a good feature of this proposal as well, where we reduce the amount or burden that these errors would mean into a more useful value.

For example, an application crashing with undefined and nothing more in the terminal is way worse than having an error like the following is much more useful in all meanings:

NullableError: Caught false error value
    at REPL20:1:1
    at ContextifyScript.runInThisContext (node:vm:136:12)
    at REPLServer.defaultEval (node:repl:598:22)
    at bound (node:domain:432:15)
    at REPLServer.runBound [as eval] (node:domain:443:12)
    at REPLServer.onLine (node:repl:927:10)
    at REPLServer.emit (node:events:531:35)
    at REPLServer.emit (node:domain:488:12)
    at [_onLine] [as _onLine] (node:internal/readline/interface:417:12)
    at [_line] [as _line] (node:internal/readline/interface:888:18) {
  value: ''
}
@crazytonyi

This comment was marked as outdated.

@bmeck
Copy link

bmeck commented Aug 21, 2024

I'd make a few arguments for an instance of some object type.

  1. Could in the future vary the object type like AggregateError expanded Error; it is possible { error, expectedError, value } could be setup in a way to help deal with the dreaded untype catch problem in TS potentially. (either via Duck or nominal typing). Would want some input from TS team on this though to confirm.
  2. Use of getter/setters from a shared prototype avoids extraneous allocation.

Even w/o TS the ability to extend Iterables (not tuples per JS spec nomenclature!) is more limited and requires iteration of all keys to get to the value desired, compare:

let foo = () => { /*...*/ ; return undefined; }
let obj ?= foo()
if (obj.threw) {
  // ...obj.error
}
let [err, value] ?= foo()
if (err) {
  // potentially was thrown for falsy values
}

Helpers for specific case detection would not really have API space in the iterator form.

@sergiohdljr
Copy link

aesthetically I prefer tuple

@crazytonyi
Copy link

aesthetically I prefer tuple

That's interesting, because I find them aesthetically unpleasant. There have been arguments throughout for why they are functionally better for this purpose, so I'm not saying they are wrong, but aesthetically, they look dumb to me. Because they assume what the operation/expression will return. It's like a fun "let's just see if this works" style of setting variables. Maybe this is part of what I find disjointed about the entire proposal, is that deconstructed assignment doesn't look stable or sensible to me (from a distance).

I'm sure they'll carve all of my personal opinions about coding on my tombstone. But as long as you bring up what we prefer aesthetically, I figured I'd chime in.

@sergiohdljr
Copy link

aesthetically I prefer tuple

That's interesting, because I find them aesthetically unpleasant. There have been arguments throughout for why they are functionally better for this purpose, so I'm not saying they are wrong, but aesthetically, they look dumb to me. Because they assume what the operation/expression will return. It's like a fun "let's just see if this works" style of setting variables. Maybe this is part of what I find disjointed about the entire proposal, is that deconstructed assignment doesn't look stable or sensible to me (from a distance).

I'm sure they'll carve all of my personal opinions about coding on my tombstone. But as long as you bring up what we prefer aesthetically, I figured I'd chime in.

i kinda see what you tryna say, but the same apply for a object, maybe your problem are with destructuring

@felippe-regazio
Copy link

felippe-regazio commented Aug 22, 2024

I think it would be safer to use an exotic object or constructor like Error or Response due the positional nature of "tuples". I don't know... SafeOperation?

  • extensible
  • easily testable/mockable
  • instant TS compatibility
const n: SafeOperation ?= fn();

Where:

type SafeOperation = { error, result };

Allowing:

const { error, result } ?= fn(); // SafeOperation type/constructor infered by the operator

I think an exotic object also does make sense since JS doesn't really have tuples (just another exotic object: Array) that can lead to strange behaviors (#5 (comment)). SafeOperation In the future can extend its own properties depending on the context, e.g.: Error which may have a response property in a network operation context (not the case of the context here, but just for the sake of the example. Something more appropriated would be: #6 (comment)).

Also as I mentioned, would be cool to use it like a Error or Response constructor:

const n = new SafeOperation(error, result); // constructs a new SafeOperation object which error and result properties

Or

const n = new SafeOperation(fn()); // constructs a new SafeOperation resultant of the fn();

@garretmh
Copy link

I'm drawn to the idea of wrapping non-Error thown values in a dedicated Error and returning a tuple. I think it would be a very nice way to deal with sync errors. I just can't see the TC39 committee going for it. I'm not exactly sure why but something about coercing thrown values doesn't feel JavaScript.

@khaosdoctor
Copy link

Even though I aesthetically prefer the object, I would go with tuples for a few reasons:

  1. This is already a known standard (from React useState)
  2. Tuples allow you to change the names of the error and data properties as you wish without the need of explicitly stating the original: { error: errObject, result: fetchResult } vs [errObject, fetchResult]

However, to @felippe-regazio's comment, I think it's also a possibility to think, either adding it as Result like Entries does for Maps and Sets, but instead of retuning the object, it returns a tuple.

@TheUniqueMathemagician
Copy link

Exactly, I also think that tuples could be a nicer option than using object destructuring because this lets people re-assign value names, thus allowing the following pattern to emerge:

const [authorsError, authors] ?= await fetch("/authors")

if (authorsError) { /* handle error */ }

const [booksError, books] ?= await fetch("/books")

if (booksError) { /* handle error */ }

@ljharb
Copy link

ljharb commented Aug 25, 2024

const { error: authorsError, result: authors } ?= await fetch('/authors');

const { error: booksError, result: books } ?= await fetch('/books');

@garretmh
Copy link

One thing I keep thinking about is this existing pattern for async error handling. Something like this for synchronous errors would be really nice.

const books = await fetch("/authors").catch((error) => {
  /* handle error */
})

@arthurfiorette
Copy link
Owner Author

await fetch("/authors").catch((error) => {

The problem with the above pattern is that it mixes the success and rejection into the same variable, making it hard to differentiate afterwards in case a default value could not be provided.

@khaosdoctor
Copy link

khaosdoctor commented Aug 26, 2024

const { error: authorsError, result: authors } ?= await fetch('/authors');

const { error: booksError, result: books } ?= await fetch('/books');

Yeah that's my point number 2, we have to repeat the original names on both objects to change the names whereas with tuples we could use:

const [authorsError, authors] ?= await fetch('/authors');
const [booksError, books] ?= await fetch('/books');

Objects are indeed more explicit, which is nice, but I think that, in this particular case, it's already explicit in the construct (especially if we use try)

@mindplay-dk
Copy link

const [reqErr, response] = try await fetch(url);

This is beautiful syntax 😄❤️

It's much more readable and self-explanatory than the cryptic two-for-one ?= operator.

@TheUniqueMathemagician
Copy link

const [response, error] = try await fetch(url);

This syntax is optimal as it aligns well with the throw expression proposal, which is currently in Stage 2. The proposed syntax is self-explanatory, making error handling more concise and readable.

@Arlen22
Copy link

Arlen22 commented Sep 14, 2024

await fetch("/authors").catch((error) => {

The problem with the above pattern is that it mixes the success and rejection into the same variable, making it hard to differentiate afterwards in case a default value could not be provided.

I work with async functions a lot and if I can't return a default value in a catch statement then I either throw an error or return undefined. If I return undefined, I can just check for that with an if statement and gracefully exit the parent function. Usually the catch is where I handle the error and if I can't handle it there I probably can't handle it anywhere. So it's essentially the same thing.

It's doubtful that I would use try expressions for most async functions since the async...await syntax already handles it perfectly, but it would be a useful shortcut for some of the typing shenanigans that can result.

@arthurfiorette
Copy link
Owner Author

arthurfiorette commented Sep 15, 2024

Throwing undefined or null, although it can be considered anti pattern, can and will happen.

now we just need find a way to handle it in this operator. we could [ok, error, data] or make error be a TypeError when the thrown error is undefined or null

@ljharb
Copy link

ljharb commented Sep 15, 2024

The latter is not an option; if a value is thrown, that's the error value, full stop.

@Arlen22
Copy link

Arlen22 commented Sep 21, 2024

I think if we did that it should be [error,data,ok]. But the error value needs to be returned exactly as-is.

The only other option would be to return a TryError with the error as a property whenever an error gets thrown. The TryError stacktrace would be to the point of the try keyword, and the error property would contain whatever value was thrown by the code (usually an Error instance).

@mindplay-dk
Copy link

Just my $.02, but why design for an anti-pattern? Throwing a value is unusual and very bad practice.

I would not suggest reusing TypeError however - introduce a new Error subtype instead. There are many already, and none of them exactly fit the bill.

I would suggest adding a new Error named e.g. ValueError or UntypedError and make it have an untyped .value property.

This will make the return type simpler, e.g. [error,data] and makes it possible to narrow the error type of Error|undefined in TypeScript, such that, by using the try operator, you are guaranteed an Error instance with a proper stack-trace, etc.

This would also enable improvements to test frameworks and error handlers, which are currently unable to obtain a stack-trace from bad code that throws a value.

Fix a problem, rather than propagating it.

@DScheglov
Copy link

@mindplay-dk

Actually it makes sense to throw an error if try expression caught nullish (or falsy) value.
But with non-Error could be issues, because some errors could be originated in another realms,
and the err instanceof Error returns a false negative result.

Additionally we can get an issue with test-runners: jestjs/jest#2549

@mindplay-dk
Copy link

Actually it makes sense to throw an error if try expression caught nullish (or falsy) value.

You want the try expression to throw an error? It's intended for catching errors.

This would make no sense to me.

But with non-Error could be issues, because some errors could be originated in another realms, and the err instanceof Error returns a false negative result.

What I'm suggesting is, the try operator would convert a non-Error into an Error subtype, such that err instanceof Error would always return true - or rather, so you wouldn't need to type-check it in the first place, because it'll always be an instance of Error.

Additionally we can get an issue with test-runners: jestjs/jest#2549

What issue? (what you linked to is something about type-checking arrays?)

Test runners wouldn't need to use the try operator, they can use it if it simplifies things for them - a regular try statement (and manual type-checking) is of course still an option. I'm not suggesting any change to the try statement.

@DScheglov
Copy link

@mindplay-dk

Actually it makes sense to throw an error if try expression caught nullish (or falsy) value.

You want the try expression to throw an error? It's intended for catching errors.

oh! My bad, I ment to return the one in this case.

What I'm suggesting is, the try operator would convert a non-Error into an Error subtype, such that err instanceof Error would always return true - or rather, so you wouldn't need to type-check it in the first place, because it'll always be an instance of Error.

err instanceof Error can return false negative result. Meaning it can return false for error, if this error has been originated in another REALM (iframe or nodejs vm)

We can wait for the Error.isError proposal to resolve this.
However it is better to affect the flow as less as possible, in this meaning to return explicit discriminator is the most robust option.
To wrap only nullish values -- the second one, to wrap the falsy values -- third and to wrap everything that is not an error -- the last one.

To wrap falsy values could be safer because it will allow to check if (error), when wrapping only nulish values requires at least weak comparision: if (error != null)

Additionally we can get an issue with test-runners: jestjs/jest#2549

What issue? (what you linked to is something about type-checking arrays?)

Oh, the #2549 is too large ) But it is the root one. You can consider smaller
jestjs/jest#15269

@DScheglov
Copy link

DScheglov commented Sep 24, 2024

Error constructor has an options argument where you can pass an object with cause property that is the original error object.

yes, it has.

But actually, the cause is not only an original error. It also could be any other data.
As instance MDN also offers to pass some structured data:

Providing structured data as the error cause

But the originalError is not an optional and it is exactly what has been caught and wrapped to be safely returned.

@garretmh
Copy link

The first lines of the Error: cause MDN page you linked to:

The cause data property of an Error instance indicates the specific original cause of the error.

It is used when catching and re-throwing an error with a more-specific or useful error message in order to still have access to the original error.

It's primary intended use is specifically wrapping a thrown value in an Error object.

It can also be used for adding additional custom context to an Error but that doesn't invalidate it as the correct property for wrapping a throw value.

@arthurfiorette
Copy link
Owner Author

arthurfiorette commented Sep 25, 2024

I've been playing with the type for a while and have been liking it.

type Result<T> = [ok: true, error: null, data: T] | [ok: false, error: unknown];

I know most of people here is not even trying the pasted examples or only trying in sample codes, but the above seemed very ugly and unpleased to work in my mind but after trying it a bit I have to say that its awesome:

// example:
const [ok, cause, data] = await fn()

if (!ok) {
  Sentry.captureException(cause)
  throw httpErrors.badRequest('Error!')
}

// continue service code.

It does not let error handling implicit ok variable. It does not breaks when undefined/null is thrown (ok) variable.

What do you guys think?

(Please try testing it out for about 1-2 days before actually having a opinion, how many JS features you didn't have a good first impression but ended up actually liking it after using it?)

@DScheglov
Copy link

The first lines of the Error: cause MDN page you linked to:

The cause data property of an Error instance indicates the specific original cause of the error.
It is used when catching and re-throwing an error with a more-specific or useful error message in order to still have access to the original error.

It's primary intended use is specifically wrapping a thrown value in an Error object.

It can also be used for adding additional custom context to an Error but that doesn't invalidate it as the correct property for wrapping a throw value.

@garretmh, @crazytonyi

ok, you win ) Let it be cause. I hope we will not need to wrap errors at all. The explicit discriminator is better

@DScheglov
Copy link

@arthurfiorette

It does not let error handling implicit ok variable.

Only for Typescript. For JS the issue with implicit error ignoring is still present.

@arthurfiorette
Copy link
Owner Author

how,?

@DScheglov
Copy link

@arthurfiorette

how,?

const [,, fileName] = try await getFilename();

fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');

@arthurfiorette
Copy link
Owner Author

For me, this is not implicit at all. [,, is screaming that something was done wrong. Again, this is not implicit also because there's the try operator.

@DScheglov
Copy link

For me, this is not implicit at all. [,, is screaming that something was done wrong. Again, this is not implicit also because there's the try operator.

yes, the old discussion.

Ok, let's look on the following case:

const [ok, error, fileName] = try await getFilename();

fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');

Are something screaming here? Actually TS also accepts this broken code

@arthurfiorette
Copy link
Owner Author

let filename

try {
  filename = await getFilename()
} catch(error) {
  console.log(error)
}

fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');

Will also pass on TS checks but a lot of linters have rules to prevent this problem. Even though only string, number, boolean + (objects with symbol to string tag) should work fine when being concatenated, however TS accepts anything:

let filename /* undefined */

if (conditionThatNeverMets) {
  filename = 'something'
}

fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');

I see that the above is a problem because fileName can be undefined but this is an instinct problem in JS land that can happen in a lot of different places as well, its correlated with the bad usage of try expressions but not only caused by it. And I don't think we have an actual solution to that, in the same way as if someone writes a try/finally it wont caught errors even though there's a try keyword there. Maybe its a problem for a linter to solve, the same way:

const [ok, error, fileName] = try await getFilename();

if (!ok) {
  console.log(filename.toString())
}

Should be a problem for TS to solve (which is somewhat a linter os steroids xd)

@DScheglov
Copy link

DScheglov commented Sep 25, 2024

@arthurfiorette

yes, there are a lot of ways to shoot yourself ...

But with try catch the code looks like the following:

try {
  const fileName = getFilename();
  fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');
} catch (err) {
  console.error(err);
}

and this a most expected way to write the code.

@arthurfiorette
Copy link
Owner Author

Not necessarily, the main reason for why this proposal was created is for this super common usecase:

let fileName;

try {
  fileName = getFilename();
} catch (err) {
  console.error(err);
}

// do a lot more stuff (and a lot more possible try/catch/finally)

fs.writeFileSync(`__dirname/${fileName}`, 'Hello World');

Where you need more than 1 try catches (so you can distinguish where the error is coming from) and maybe to write more code after the handlings and only then do what you want to do with the original file.

I don't think this proposal is aiming to replace try blocks the same way let replaced var usage almost everywhere. Your example above is a fine use case to be kept with try catch blocks.

For example, this below code (thanks chatgpt) is very common inside business code:

function processData(data: string) {
  try {
    console.log("Outer try block");

    // Simulate a potential outer error
    if (!data) throw new Error("Outer Error: No data provided");

    try {
      console.log("Middle try block");

      // Simulate a parsing error
      const parsedData = JSON.parse(data);
      console.log("Parsed Data:", parsedData);

      try {
        console.log("Inner try block");

        // Simulate a validation error
        if (!parsedData.valid) throw new Error("Inner Error: Invalid data");

        console.log("Data is valid:", parsedData);
      } catch (innerError) {
        console.error("Caught in inner catch:", innerError.message);
      }

    } catch (middleError) {
      console.error("Caught in middle catch:", middleError.message);
    }

  } catch (outerError) {
    console.error("Caught in outer catch:", outerError.message);
  }
}

where either you use multiple nested catches to handle different errors or you only use one and isn't able to distinguish where specifically the error was being thrown.

And using this approach gives a much more readable verison: (thanks chatgpt again)

function processData2(data: string) {
  console.log('First step block');

  // Simulate a potential outer error
  if (!data) throw new Error('Outer Error: No data provided');

  console.log('Middle try block');

  const [ok, error, parsedData] = try JSON.parse(data);

  if (!ok) {
    console.error('Caught in middle catch:', error.message);
    return;
  }

  console.log('Parsed Data:', parsedData);

  // Simulate a validation error
  const [ok, innerError, valid] = try validateData(parsedData);

  if (!valid) {
    console.error('Caught in inner catch:', innerError.message);
    return;
  }

  console.log('Data is valid:', parsedData);
}

@mindplay-dk
Copy link

type Result<T> = [ok: true, error: null, data: T] | [ok: false, error: unknown];

What do you guys think?

@arthurfiorette this still leaves a bad taste in my mouth - I know the ok flag isn't exactly duplicating error, but it's very close, except for the ugly marginal edge case where some evil genius throws a null or undefined.

I just don't like the idea of adding complexity to design/optimize for (or even cope with) an anti-pattern - remembering the exact order of those 3 return values is going to be the sort of thing I'd have to look up on a daily basis, I think. 😔

I still think it would make more sense to wrap non-Error values in an Error instance. The engine knows at the throw-statement what's being thrown - it can capture a stack-trace internally (which it does already for uncaught errors) and provide it via the Error-instance to the try-operator, which would be an improvement overall, and probably something test-frameworks would want to use, so they can provide a stack-trace for errors where currently no stack-trace is captured.

@DScheglov based on your comment here I think you misunderstood me the second time again - I am not suggesting every error be wrapped. Only non-Error values would be wrapped, in an Error-subtype with a value property. You won't have nested errors. You will have just have an Error where previously you had a non-Error value. (You also won't have any change to regular try/catch statements.)

@DScheglov
Copy link

@arthurfiorette

the exceptions are expected to be used in the way to get the proccessData function as follows:

function processData(data: string) {
  try {
    validateInput(data);
    const parsedData = parseJson(data);
    return validateData(parsedData);
  } catch (err) {
    if (handleDataProcessingError(err)) return false;
    throw err;
  }
}

See in Playground

Actually your refactored with try-expression code compiles with errors:

console.error('Caught in middle catch:', error.message);
//                                       ^^^^^  'error' is of type 'unknown'.
 console.error('Caught in inner catch:', innerError.message);
 //                                      ^^^^^^^^^^ 'innerError' is of type 'unknown'.

And worning:

  const [ok, innerError, valid] = try validateData(parsedData);
  //     ^^ 'ok' is declared but its value is never read.

see in Playground

In the real word knowning where the error is caught means nothing. More then, trying to conclude any decision basing only on the place of caught could be dangerous -- you examples shows that. TS founds the error! JS doesn't.

More then, because we have unknown type for the error, we don't have a worning, that error is possibly undefined.
Let's guess, the validateData in some moment will return false instead of throwing an exception -- the code becomes broken

@DScheglov
Copy link

DScheglov commented Sep 25, 2024

@mindplay-dk

@DScheglov based on your comment #30 (comment) I think you misunderstood me the second time again - I am not suggesting every error be wrapped. Only non-Error values would be wrapped, in an Error-subtype with a value property. You won't have nested errors. You will have just have an Error where previously you had a non-Error value. (You also won't have any change to regular try/catch statements.)

I've never told that you suggested to wrap everything we caught. ) It did @Arlen22 in the comment#2371679490

I still think it would make more sense to wrap non-Error values in an Error instance. The engine knows at the throw-statement what's being thrown - it can capture a stack-trace internally (which it does already for uncaught errors) and provide it via the Error-instance to the try-operator, which would be an improvement overall, and probably something test-frameworks would want to use, so they can provide a stack-trace for errors where currently no stack-trace is captured.

Let's guess we have some evil framework, where we call a framework api inside of our code, and the framework calls our code.
This framework expected to catch a thrown by its api Promise or null, or undefined ... -- wrapping non-Errors in this case will automatically results:

  • we cannot use try-expression with the framework
  • we cannot use the framework because it throws non-error values.

Yes, it is an anti-pattern, but it is allowed by ES and TS. It is better to find the solution that doesn't break an existing code.

@DScheglov
Copy link

@arthurfiorette

even the JSON.parse can throw not-own error:

const none = {
  toString() {
    throw new ReferenceError('Attemt to resolve none as a string')
  },
};

console.log(
  JSON.parse(none as any)
);

Playground

So, never, never, and one more time never rely on the place where you caught the exception.

There is another entity that allows us to rely on the "place" where it is received.
It is a return value.
Meet Rust's Result<T, E>.

@mindplay-dk
Copy link

@DScheglov it still sounds like you don't understand what I'm suggesting.

Let's guess we have some evel framework, where we call a framework api inside of our code, and the framework calls our code.
This framework expected to catch a thrown by its api Promise or null, or undefined ... -- wrapping non-Errors in this case will automatically results:

  • we cannot use try-expression with the framework
  • we cannot use the framework because it throws non-error values.

Neither of those things would happen.

If a non-Error value is thrown, it isn't wrapped at the throw site - that would be a breaking change.

It would get wrapped at the try expression site, immediately before it gets returned - so this only affects the try expression. From there, if you need to rethrow, you could choose to throw the value again, or re-throw the Error-subtype wrapper.

(Either way, it would be an extremely strange framework or error-handler that only expects and handles non-Error values.)

@TheUniqueMathemagician
Copy link

TheUniqueMathemagician commented Sep 25, 2024

I'm not really a fan of the "ok" syntax; it makes the code unnecessarily verbose for handling edge cases. I would prefer using the try expression as a day-to-day feature because it has a more friendly syntax than having to destructure three values.

When discussing the order of values, I believe having the result first is more practical. In everyday work, a result that equals null typically indicates something went wrong, which is sufficient information to handle the case (yes, it means discarding the error, but hey, I'm talking about how people will use the feature, not what it should be 😜). So the [,,result] syntax will be seen very often I'm affraid, like people handling errors only by writing console.log in their catch clause (yes, I've seen that a lot, in many companies). I mean, you could try to force them to use the error by placing it as a first value in the returned tuple, but we all know what they'll do 👀

I like to compare this feature to promises and async code. An expression could either succeed and return a value or throw an error, similar to how promises work with (resolve, reject). Having only [result, error] is much more explicit and familiar for those encountering this feature for the first time.

Also, for example:

let result;
try {
    result = await fetch("https://api.example.com");
} catch (error) {
    return toast("Something went wrong");
}
// Do something with the result

This could be simplified to:

const [result] = try await fetch("https://api.example.com");
if (!result) return toast("Something went wrong");
// Do something with the result

The second solution:

  • Is much shorter, which usually means more maintainable, more readable, and thus less prone to logic errors, making it more productive.
  • Avoids declaring multiple scopes and nesting them.
  • Provides a way to discard the error value if we don't care about its actual content.
  • Assigns the value to a const declaration, which prevents editing the value further on.

I'm also convinced that the error should be the exact same as the error thrown by the try await clause. The try await should be a simple, transparent JavaScript feature that doesn't alter the behavior or the API of what was thrown. Don't let the feature do more than what it is intended for.

Though, I do agree that this code makes JavaScript look a bit like Go! 😄

@DScheglov
Copy link

@mindplay-dk

I got you correctly

_try is kind of polyfill for try-expression and it looks like as follows:

export const _try: {
  (fn: () => never): Failure;
  (fn: () => Promise<never>): Promise<Failure>;
  <T>(fn: () => Promise<T>): Promise<ResultTuple<T>>;
  <T>(fn: () => T): ResultTuple<T>;
} = <T, >(fn: () => T | Promise<T>): any => {
  if (typeof fn !== 'function') {
    return failure(new TypeError('Parameter fn is not a function', { cause: fn }));
  }

  try {
    const result = fn();
    return isPromise(result)
      ? result.then(success, failure)
      : success(result);
  } catch (error) {
    return failure(error);
  }
};

Where the success and failure are defined as:

export type Failure = readonly [error: Error];
export type Success<T> = readonly [error: undefined, value: T];
export type ResultTuple<T> = Failure | Success<T>;

const success = <T, >(value: T): Success<T> => [undefined, value] as const;
const failure = (err: unknown): Failure => [TryError.ensureError(err)] as const;

And finally the TryError class:

class TryError extends Error {
  static ensureError(value: unknown): Error {
    return Error.isError(value) ? value : new TryError(value);
  }

  constructor(public readonly cause: unknown) {
    super('Non-Error has been thrown.', { cause })
  }
}

See Playground

It would get wrapped at the try expression site, immediately before it gets returned - so this only affects the try expression. From there, if you need to rethrow, you could choose to throw the value again, or re-throw the Error-subtype wrapper.

So, the pointed above solution affects already existing code

Let's suppose we have some framework, that allows us to write the following code:

type UseFn = <T>(fn: () => Promise<T>) => T;

const HelloComponent = (use: UseFn) => {
  const name = use(() => Promise.resolve('World'));

  return `<h1>${name}</h1>`
}

To render the component HelloComponent we have a function render:

async function render(
  fnComponent: (useFn: UseFn) => string,
) {  
  let useCounter = 0;
  const useValues: unknown[] = [];

  function use<T>(fn: () => Promise<T>): T {
    if (useValues.length <= useCounter) throw fn();
    return useValues[useCounter++] as T;
  }

  const run = async () => {
    try {
      useCounter = 0;
      return fnComponent(use);
    } catch (err) {
      if (isPromise(err)) {
        useValues[useCounter] = await err;
        return run();
      }
      throw err;
    }
  }

  return run();
}

And rendering the Component:

render(HelloComponent).then(console.log);

Now, let's change the const name = use(... to const name = try use(...:

const HelloComponent = (use: UseFn) => {
  const [error, name] = _try(
    () => use(() => Promise.resolve('World'))
  );

  if (error) {
    console.log('We got an error', error);
    throw error;
  }

  return `<h1>${name}</h1>`
}

We will get an error.

Just because we are wrapped the Promise with error.

Full Example

Why am I talking about this affects an existing code.
Fast of all the framework mantainers will receive a lot of issues, that it is not possible to use new syntax with the framework,
and what will they do? Exactly they will add the correspondent case in the catch block in render function:

    } catch (err) {
+      if (err instanceof Error && isPromise(err.cause)) {
+       useValues[useCounter] = await err.cause;
+        return run();
+      }
      if (isPromise(err)) {

So, despite we are using new syntax only in new code, we have to update an existing as well.

@DScheglov
Copy link

@TheUniqueMathemagician

it looks like you need another kind of expression:

const response = try? await fetch(...); 
// or
const response = try! await fetch(...);

if (response == null) ...

In general, to ignore error -- is an anti-pattern, so it is not a good idea to encourage this by introducing new "concise" syntax.

@zaygraveyard
Copy link

zaygraveyard commented Sep 26, 2024

@arthurfiorette The example you gave can very easily be rewritten in a cleaner way (since errors can't escape the catch block unless rethrown):

function processData(data: string) {
  try {
    console.log("Outer try block");

    // Simulate a potential outer error
    if (!data) throw new Error("Outer Error: No data provided");

    try {
      console.log("Middle try block");

      // Simulate a parsing error
      const parsedData = JSON.parse(data);
      console.log("Parsed Data:", parsedData);

      try {
        console.log("Inner try block");

        // Simulate a validation error
        if (!parsedData.valid) throw new Error("Inner Error: Invalid data");

        console.log("Data is valid:", parsedData);
      } catch (innerError) {
        console.error("Caught in inner catch:", innerError.message);
      }

    } catch (middleError) {
      console.error("Caught in middle catch:", middleError.message);
    }

  } catch (outerError) {
    console.error("Caught in outer catch:", outerError.message);
  }
}

Becomes:

function processData(data: string) {
  try {
    console.log("Outer try block");

    // Simulate a potential outer error
    if (!data) throw new Error("Outer Error: No data provided");
  } catch (outerError) {
    console.error("Caught in outer catch:", outerError.message);
  }

  let parsedData; // the `let` can become `const` if the [write-once const proposal](https://github.com/nzakas/proposal-write-once-const/) is accepted
  try {
    console.log("Middle try block");

    // Simulate a parsing error
    parsedData = JSON.parse(data);
    console.log("Parsed Data:", parsedData);
  } catch (middleError) {
    console.error("Caught in middle catch:", middleError.message);
  }

  try {
    console.log("Inner try block");

    // Simulate a validation error
    if (!parsedData.valid) throw new Error("Inner Error: Invalid data");

    console.log("Data is valid:", parsedData);
  } catch (innerError) {
    console.error("Caught in inner catch:", innerError.message);
  }
}

Plus the processData2() version you gave as example doesn't match the behavior of the processData(), so here's processData2() but with try .. catch:

function processData2(data: string) {
  console.log('First step block');

  // Simulate a potential outer error
  if (!data) throw new Error('Outer Error: No data provided');

  console.log('Middle try block');

  let parsedData; // the `let` can become `const` if the [write-once const proposal](https://github.com/nzakas/proposal-write-once-const/) is accepted
  try {
    parsedData = JSON.parse(data);
  } catch (error) {
    console.error('Caught in middle catch:', error.message);
    return;
  }

  console.log('Parsed Data:', parsedData);

  // Simulate a validation error
  try {
    const valid = validateData(parsedData);
    if (!valid) throw new Error("Inner Error: Invalid data");
  } catch (innerError) {
    console.error('Caught in inner catch:', innerError.message);
    return;
  }

  console.log('Data is valid:', parsedData);
}

@DScheglov
Copy link

@zaygraveyard

In your first snippet, the parsedData is not accessible in the next try block.

This is exactly a case for the try expression -- to achieve a "flat" flow without
using a let declaration (as in your second snippet), mostly to get a single point
where the variable's value is defined.

@zaygraveyard
Copy link

@DScheglov Thanks for catching that small oversight, its fixed now 😄

@zaygraveyard
Copy link

@DScheglov And as I mentioned in the code snippet, the let can be replaced by const if/when write-once const proposal is accepted.

Can you explain what you mean by a "flat" flow?

@DScheglov
Copy link

DScheglov commented Sep 26, 2024

@zaygraveyard

@DScheglov And as I mentioned in the code snippet, the let can be replaced by const if/when write-once const proposal is accepted.

Can you explain what you mean by a "flat" flow?

"Flat" -- without nested "try"-s

About the write-once const proposal: it is in the "Stage: -1".
Let's consider it at least on the "Stage: 0".
We also have a "do"-expression proposal, it is in the Stage 1.

@zaygraveyard
Copy link

I'm not arguing against the do-expression proposal nor against this one.
I'm just trying to weigh the pros/cons or cost/benefit of this proposal.

The write-once const proposal is much more limited in scope and I believe has more benefit.

It's still unclear to me what exactly is the problem this proposal is trying to solve (I asked on multiple occasion but got no answer)...
A list of use cases showing problems and how this proposal solves them would be grand 😄

@DScheglov
Copy link

DScheglov commented Sep 26, 2024

@zaygraveyard

It's still unclear to me what exactly is the problem this proposal is trying to solve (I asked on multiple occasion but got no answer)...

That’s a great observation.

What I’m noticing is that some developers prefer to have a default nullish value in case of an unsuccessful computation. Typically, these developers also favor the data-first tuple approach.

The popularity of such approach we can see on how many downloads the nice-try library has: ~13.5M/week:

const [data] = try JSON.parse(input); // data undefined or parsed json

So, it is definitelly the case, that must be considered for the proposal.

The second one is pointed by @arthurfiorette's comment#2374175054:

  • get rid of nested try-s without using the let declarations.

The most popular lib for that (that I found) is safely-try with ~9k/week downloads.
But the safely-try doesn't correctly support falsy values thrown as exceptions, so its usage can lead to bugs.

The difference between these two cases: 1.5k times!!!

The reason for this difference could be explained by the findings from a survey conducted by the TypeScript team regarding exception usage:

Generally speaking, JS code goes into exactly two paths: the happy case, or the "something went wrong" case, in which meaningful introspection as exactly what went wrong is quite rare.

Source: #13219 (commnet)

cc: @arthurfiorette

UPD:
I found more popular library for the second approach: try-catch. It has ~57k downloads per week.
Doesn't effect the consideration

@zaygraveyard
Copy link

@DScheglov

I agree that "default nullish value in case of an unsuccessful computation" can be useful in some cases (I have multiple those most projects), but it feels to me like it should always be explicitly clear that the error is being ignored (which this proposal doesn't yet enforce).

As for the "get rid of nested try-s without using the let declarations", the write-once const proposal also solve that problem and with a much more limited scope.

BTW, I'm all for the "do"-expression proposal and would love to see it added to JS 😄

@DScheglov
Copy link

@zaygraveyard

I agree that "default nullish value in case of an unsuccessful computation" can be useful in some cases (I have multiple those most projects), but it feels to me like it should always be explicitly clear that the error is being ignored (which this proposal doesn't yet enforce).

100%! Joining to this. But the Champion of the proposal is disagree with that even error-first approach

const [, data] = try JSON.parse(input);

is an implicit error ignoring.

So, it looks very debatable issue.

As for the "get rid of nested try-s without using the let declarations", the write-once const proposal also solve that problem and with a much more limited scope.

I'm not sure about that. I'd prefer to avoid discussing the "write-once const" here, but if this proposal is implemented, we might end up with one place where the variable is declared and multiple places where it is assigned a value, which could make the code harder to read and understand. In this sense, let seems more honest.

BTW, I'm all for the "do"-expression proposal and would love to see it added to JS

I guess it makes sense to extend the "do"-expression proposal with adding a catch block after the do-block:

const data = do { Parse.json(input) } catch { null };

It looks much more concise then:

const data = do {
  try { Parse.json(input) } catch { null }
}

@zaygraveyard
Copy link

@DScheglov

I also agree that we should avoid discussing other proposals here, but we also can't discuss this proposal in isolation when other proposals might solve the same problems. 😅

But just to clarify, the "write-once const" proposal would only allow detaching the initialization of a const from its declaration, while still forbidding any assignment to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests