Some gentle criticism of this proposal

[nr-goby]

I've been looking over the proposal and I'm not quite convinced that it is necessary. Let's look at your code example:

async function handle(request, reply) {
  const userInfo = try await cache.getUserInfo(request.id)

  if (!userInfo.ok) {
    logger.error(userInfo.error, "Maybe DB is down?")
    return reply.status(500).send({ error: "Could not get user info" })
  }

  const posts = try await db.getPosts(userInfo.value.authorId)

  if (!posts.ok) {
    logger.error(posts.error, "Anonymous user behavior not implemented yet")
    return reply.status(500).send({ error: "Could not get posts" })
  }

  const comments = try await db.getComments(posts.value.map((post) => post.id))

  if (!comments.ok) {
    logger.error(comments.error, "Posts without comments not implemented yet")
    return reply.status(500).send({ error: "Could not get comments" })
  }

  // No need for reassignable variables or nested try/catch blocks

  // Do something with comments before returning
  return reply.send({ userInfo: userInfo.value, posts: posts.value, comments: comments.value })
}

I believe you can easily achieve this in Javascript as of today in an alternate way, let me show you:

async function handle(request, reply) {
  // userInfo: User | void
  const userInfo = await cache
    .getUserInfo(request.id)
    // since our logger returns void, we can use that value as our reciprocal value to branch on
    .catch((err) => logger.error(err, "Maybe DB is down?"));

  // now we can check on existence and return our reply status, could also be userInfo !== undefined
  if (!userInfo)
    return reply.status(500).send({ error: "Could not get user info" });

// userInfo now narrowed to User
// userInfo: User 

// posts: Post[] | void
  const posts = await db
    .getPosts(userInfo.authorId)
    .catch((err) =>
      logger.error(err, "Anonymous user behavior not implemented yet")
    );

  if (!posts) return reply.status(500).send({ error: "Could not get posts" });

// comments: Comment[] | void
  const comments = await db
    .getComments(posts.map((post) => post.id))
    .catch((err) =>
      logger.error(err, "Posts without comments not implemented yet")
    );

  if (!comments)
    return reply.status(500).send({ error: "Could not get comments" });

  // No need for Result objects to complicate our logic

  return reply.send({
    userInfo,
    posts,
    comments,
  });
}

I know it's just a small example, but you gave an example that uses only promises and promises have pretty good error handling built into them. Can you maybe show another example that your proposal would provide a better solution for?

[ottobunge]

I think I might not be understanding something correctly because the way I see your suggestion there's no way to differentiate between an undefined because of an error and an undefined because of a not found resource.

Also there's the case where you need to do something else with the error, you would need to do some very unergonomic things to use it.

Something like:

const maybeError: someError | null = null;
const userInfo = await cache
    .getUserInfo(request.id)
    // since our logger returns void, we can use that value as our reciprocal value to branch on
    .catch((err) => {
        maybeError = err;
    });

[Arlen22]

@nr-goby you are correct that this proposal is somewhat unnecessary when it comes to Promises (which all of your examples above are), as you can literally use the same pattern as we are proposing.

const [good, error, value] = await fetch(...)
  .then(e => [true, undefined, e], e => [false, e, undefined])

However, this is impossible when working with synchronous code. JSON and SQLite are the two most common examples I can think of at the moment, but anything synchronous has the same problem. The only way to catch an error is via a block statement, which then makes it much harder to get the value should the attempt succeed.

let _value; // no way to get rid of this
try {
  _value = JSON.parse(data);
} catch (e) {
  _value = undefined;
}
const value = _value; // const assignment

// much simpler
const [,,value] = try JSON.parse(data);

if(value === undefined) {
  // JSON.parse never returns undefined on success, 
  // and the error rarely matters in production,
  // so we can just check if value is undefined.
}

The async promise example also does not catch errors thrown before the promise is returned.

function getUser(user_id: string){
  ok(typeof user_id === "string");
  return fetch(...);
}

[nr-goby]

@Arlen22 You can wrap exception throwing synchronous code in the new Promise.try function. Using your example:

// we cast JSON parse to unknown to avoid collapsing type to any
// value: unknown | Error
const value = await Promise.try(() => JSON.parse(data) as unknown).catch(
  (cause) => new Error("JSONParseError", { cause })
);

if (value instanceof Error) return value;

// value is narrowed to unknown and can be validated

This is already quite flexible in terms of being able to branch on a reciprocal value. In this case, I wrapped the thrown value in the error cause which can be accessed & returned. This will contain the thrown value along with the stack trace at the exact line of the callback, making it easy to track down the error.

If you don't have access to the Promise.try static method, you can easily polyfill it yourself in typescript with some generics.

This becomes even more ergonomic when you include a validation step:

// value: Data | Error
const value = await Promise.try(() => JSON.parse(data))
    .then(data => v.parse(dataSchema, data))
    .catch(
      (cause) => new Error("JSONParseError", { cause })
);

if (value instanceof Error) return value;

Notice that our single catch statement will wrap either statement and provide us with a stack trace. All we have to do is look at the cause in order to see the error that was thrown. If you need to branch on a specific error, then you could use a custom error class and check for it. We also now don't have to cast JSON.parse to unknown anymore because we batched in the validation which will result in our wrapped error if it doesn't meet our dataSchema requirements.

[nr-goby]

@ottobunge I'm not sure why you're declaring variables above the return value of the catch. In my example, I'm returning void from the catch as an example, you can return anything you want from the catch. The point is to use that value to branch on. If you use Typescript, you'll see that the value returned is a union of our desired value, and whatever is returned from the catch. If you don't wrap the caught value, your union will collapse to any.

// you don't need to do this
//const maybeError: someError | null = null;

// userInfo: User | Error
const userInfo = await cache
    .getUserInfo(request.id)
    // wrapping the thrown value in an Error
    .catch((cause) => new Error("UserInfoError", { cause }));

if (userInfo instanceof Error) return userInfo;
// userInfo now narrowed to User

The point I'm trying to make is that you can easily change the your return type to give yourself flexibility in branching afterwards. Also notice that I didn't have to do any type casting. The code is much cleaner and all the types are now inferred by Typescript.

[ottobunge]

@nr-goby You're right, I blanked on instanceof

[Arlen22]

@nr-goby that's interesting, I had not seen Promise.try before. However I don't believe it fully satisfies the use-cases of our proposal because it still introduces a function boundary. As such it is no different than any other workaround that we already have available.

The primary benefit of the try operator is that it does not change the execution context at all, it simply catches any errors thrown by the evaluation of its expression. In JavaScript, there is no way to catch an error thrown in an expression until you're well outside of the expression context. Even a sync form of Promise.try must use a callback function, which has a number of disadvantages, especially for type inference.

[nr-goby]

@Arlen22 I guess I need help understanding why a function boundary is unacceptable? Wrapping the caught value provides type safety. Tagging those errors provides enhanced branching capability.

Also, I thought this proposal doesn't aim to provide type safe errors?

Introducing something into a runtime to avoid calling a function seems a bit extreme, no? Can you provide some real world examples where changing the execution context causes insurmountable issues?

[Arlen22]

@Arlen22 I guess I need help understanding why a function boundary is unacceptable? Wrapping the caught value provides type safety. Tagging those errors provides enhanced branching capability.

Also, I thought this proposal doesn't aim to provide type safe errors?

I don't think I said anything about error types. Nothing is changing there. If there is an error, nothing about either its types or its runtime semantics are changing at all. Not sure why you're mentioning the error type.

Introducing something into a runtime to avoid calling a function seems a bit extreme, no? Can you provide some real world examples where changing the execution context causes insurmountable issues?

It's not that we're trying to avoid calling a function, it's that we're trying to avoid putting normal code inside a function callback when there is nothing which otherwise require it. We aren't trying to avoid calling a function, we're trying to find a way to not have to use immediately invoked callbacks (which are themselves an anti-pattern) in order to catch errors in specific expressions. Since the await keyword virtually eliminated callback cascades in the async world, it seems natural to add a keyword for the one place they are still unavoidable in the sync world.

This is approaching the limits of my own JavaScript experience, as I generally deal with async code most of the time, but this reminds me of a pattern I've seen in the sync world where instead of quickly throwing an exception whenever something goes wrong, they will instead return a failure object like { success: false, reason: "message" } to normal parent code that normally would expect an exception.

Whenever I'm working with async code, I never feel the need to do this because I can easily add a catch to a promise wherever I need to capture the error and continue normally. I believe people in the sync world are scared of throwing exceptions because of how annoying they are to handle, and this is one way which will drastically improve their usefulness and ease of use.

Hopefully other people will be able to weigh in on this as I mostly deal with async code and I don't know what other anti-patterns there might be related to this in the sync world.

[nr-goby]

@Arlen22 I think the confusion about the type safety comes down to the thrown value being of any type. This causes a type collapse into any if you return a union of a value and a "naked" caught exception. This is why I said that wrapping the error provides type safety. It preserves the union & prevents the type from being collapsed into any. This allows Typescript to correctly infer the type after being narrowed by branching.

This might be a semantic issue so that can be put aside.

I'm curious about your mention that an immediate callback function is considered an anti pattern. I've only heard about excessive callbacks being an anti pattern, like in the days before promises landed in the language. Can you elaborate or provide a reputable source for it?

[Arlen22]

@Arlen22 I think the confusion about the type safety comes down to the thrown value being of any type. This causes a type collapse into any if you return a union of a value and a "naked" caught exception. This is why I said that wrapping the error provides type safety. It preserves the union & prevents the type from being collapsed into any. This allows Typescript to correctly infer the type after being narrowed by branching.

The try operator still has to return enough information for the program to determine whether an error occurred, which naturally makes it possible to correctly type the value once an error is ruled out. We never simply return a union of the value and error. There has to be an indication of whether it succeeded or failed which can be checked at runtime, and if you can check it at runtime you can type it. Please see #54 (comment), especially point 4.

[Arlen22]

I'm curious about your mention that an immediate callback function is considered an anti pattern. I've only heard about excessive callbacks being an anti pattern, like in the days before promises landed in the language. Can you elaborate or provide a reputable source for it?

No idea. I'll have to let more experienced developers weigh in on that one.

[nr-goby]

@Arlen22 I think you're misunderstanding the wrapping mechanism here. The existence of the Error is the proof that the error occurred. We're wrapping the thrown value at the error boundary and ensuring that type as an Error as it's caught. The thrown value can be accessed via the cause field of the error.

We are the bouncer as the door making sure that throw is behaving correctly. This might sound tedious but I'm not convinced that this proposal solves that situation.

edited for clarity

[DScheglov]

@nr-goby

It is a really bad idea to wrap synchronous code with Promise.try solely to handle an error.
This is undesirable for many reasons: from a performance standpoint to the actual need to execute blocking code.

The error instanceof Error check is also not a good approach. See: https://github.com/tc39/proposal-is-error

Regarding the criticism of this proposal:

Even if we take a look at your example:

  const userInfo = try await cache.getUserInfo(request.id)

  if (!userInfo.ok) {
    logger.error(userInfo.error, "Maybe DB is down?")
    return reply.status(500).send({ error: "Could not get user info" })
  }

and

  const userInfo = await cache
    .getUserInfo(request.id)
    .catch((err) => logger.error(err, "Maybe DB is down?"));

  if (!userInfo)
    return reply.status(500).send({ error: "Could not get user info" });

In the catch case, we have two places within a single function where the same error is being handled. So, if we can find a way to gracefully handle errors in a single place, it would be a benefit of the proposal.

[nr-goby]

@DScheglov

First off, being worried about the performance of a callback on error prone functions is absolutely insane. Second, Promise.try was made for this exact use case. Javascript put something in the language to do what you need and you're refusing to use it, that's your fault, nobody else's.

Also, if you read the MDN documentation I linked to Promise.try, you'd see that the callback is optional and they've taken performance into account for insane arguments like this. I'd remind you that performance is not a reason listed in this proposal.

Maybe you also didn't read my initial post but the point of it was that you need not branch on one particular thing, this approach is about flexibility. If you don't like instanceof, you can create custom errors with custom fields like tags to branch on. Do I really need to teach a course on error handling to get through here?

I feel like the last point is nitpicking, there's error things in two places, so what? Is what's going on really that confusing to you? I wrote it that way because I was adapting the example provided, not because this is the way I would do it. These are examples meant to communicate a point. The point is that you can write Javascript applications today and largely avoid try/catch, if you'd like. None of these arguments show that this proposal accomplishes something substantial that can't already be accomplished in the language in its current state.

[DScheglov]

Hey, @nr-goby

Well, I need to explain my points a little bit (before you start teaching me how to handle errors).

1. My point regarding wrapping synchronous code with Promise.try is exactly to address the following statement:

@Arlen22 You can wrap exception throwing synchronous code in the new Promise.try function. Using your example:

// we cast JSON parse to unknown to avoid collapsing type to any
// value: unknown | Error
const value = await Promise.try(() => JSON.parse(data) as unknown).catch(
  (cause) => new Error("JSONParseError", { cause })
);

if (value instanceof Error) return value;

// value is narrowed to unknown and can be validated

Not more, not less. And the problem is not in the Promise.try itself. The problem is that such a conversion is undesired for many reasons: starting from the minor reason that asynchronous code is less performant than synchronous code (see Event Loop and async/await V8 implementation notes), and ending with rare but critical cases when we need to run exactly blocking code.

But the main and most important concern regarding the sync-to-async conversion is that in real projects, we cannot just convert a function from synchronous to asynchronous because it may cause cascade changes in the project, which is definitely undesired.

So, the Promise.try is just irrelivant when we talk about error handling in synchronous code.

2. Regarding the error instanceof Error. To address the issue, you propose defining a separate error class — which is unnecessary boilerplate when using the try expression. The explicit ok discriminator solves the issue on your behalf.

const [isParsed, parsingError, parsed] = try JSON.parse(data) as unknown;

if (!isParsed) return new Error("JSONParseError", { cause: parsingError });

3. Regarding the example:

You rewrote an existing example and then used the example as an argument that this proposal doesn't "accomplish something substantial that can't already be accomplished in the language in its current state."

I pointed out that if I want to have a single place where I handle a single error, the try expression is a good way to do that, desplite it could be accomplished with existing language tools (actually it requires using something like Try function helper).

What does confuse me with your example. Let's look on it again (I've added a comments and curly braces):

  const userInfo = await cache
    .getUserInfo(request.id)
    .catch((err) => {
      // handling an error
      logger.error(err, "Maybe DB is down?");
     });

  if (!userInfo) {
    // handling the case when the userInfo is not in the cache.
    return reply.status(500).send({ error: "Could not get user info" });
  }

In the catch block, we explicitly handle the error, but in the if statement, we cover another case — when there is no userInfo in the cache. Does it cover the error we already handled in the catch block, or not? What if we allow getUserInfo() to return null or undefined if the cache doesn't contain user information for the requested ID? We have incorrect handling here. The correct approach would be expected, for example, as follows:

const userInfo = try await cache.getUserInfo(request.id);

if (!userInfo.ok) {
  logger.error(err, "Maybe DB is down?"));
  return reply.status(500).send({ error: "Could not get user info" });
}

if (!userInfo.value) {
  return reply.status(404).send({ error: "User Info is not found." });
}

And yes, there is no problem to refactor this code with promisies by introducing an additional abstraction:

const userInfo = await cache.getUserInfo(request.id).then(
  value => value != null ? { value } : { status: 404, error: "User Info is not found." },
  err => {
    logger.error(err, "Maybe DB is down?");
    return { status: 500, error: "Could not get user info" }
  });

  if (userInfo.error) {
    return reply.status(userInfo.status).send({ error: userInfo.error });
  }

But referring back to Promise.try we also had an opportunity to accomplish the same result with:

const value = await new Promise(resolve => resolve(fn()));

or with

const value = await (async () => fn())();

or even by defining a simple helper;

const promiseTry = async (fn, ...args) => fn(...args);

const value = await promiseTry(fn);

But as you can see the Promse.try has been approved and now it is a part of ECMAScript standard.