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

alternative rewrite of the motivating example with existing language features #9

Open
seansfkelley opened this issue Aug 15, 2024 · 30 comments

Comments

@seansfkelley
Copy link

As written, the motivating example

async function getData() {
  const response = await fetch("https://api.example.com/data")
  const json = await response.json()
  return validationSchema.parse(json)
}

could instead be rewritten to something along the following lines

function getData() {
  return fetch("https://api.example.com/data")
    .catch(handleRequestError)
    .then(r => r?.json())
    .catch(handleParseError)
    .then(json => json == null ? json : validationSchema.parse(json))
    .catch(handleValiationError);
}

without introducing new syntax.

It's slightly awkward with the ?. and == null checks, but it's plenty clear that it's doing a series of individually-failable operations with their own error handling (then alternating with catch), and treats null/undefined as the early-abort sentinel for downstream operations.

I don't have an alternate motivating example to provide, but I did want to raise that this one doesn't strike me as all that motivating.

@landsman
Copy link

agreed

@anacierdem
Copy link

anacierdem commented Aug 16, 2024

My general problem with the pattern in its most general form is it allows skipping an error check while not providing a compelling syntax improvement over regular error checking. Consider:

const [error, result] ?= example()

if (error) {
  // Handle error
} else {
  // Use result
}

vs:

try {
  result = example()
} catch(error) {
  // Handle error
}

There is essentially no difference for this regular case. The one with the proposed ?= OTOH allows skipping the error check altoghether by design:

const [, result] ?= example()
// use result

This leaves the code in an undefined state. Even if you don't use try/catch the code would throw and let you know the underlying error instead of "eating" it.

It is true that typescript could save the day when you don't check for error, but not everyone have to use it. This should be independent of any external type system.

Let me also try (no pun intended) to go over the important objections for regular try catch:

// Nests 1 level for each error handling block
async function readData(filename) {
  try {
    const fileContent = await fs.readFile(filename, "utf8")

    try {
      const json = JSON.parse(fileContent)

      return json.data
    } catch (error) {
      handleJsonError(error)
      return
    }
  } catch (error) {
    handleFileError(error)
    return
  }
}

This is not structured correctly, see this equivalent code (error types are made up here):

async function readData(filename) {
  try {
    const fileContent = await fs.readFile(filename, "utf8")
    const json = JSON.parse(fileContent)
    return json.data
  } catch (error) {
    if (error instanceof JSONError) {
      handleJsonError(error)
    } else if (error instanceof FileError){
      handleFileError(error)
    }
  }
}

with the proposed syntax:

async function readData(filename) {
  const [readError, fileContent] ?= await fs.readFile(filename, "utf8")

  if (readError) {
     handleFileError(error)
     return
  }

  const [jsonError, json] ?= JSON.parse(fileContent)

  if (jsonError) {
     handleJsonError(error)
     return
  }
  return json.data
}

It is almost identical with the added complexity of sprinkling error checks throught the code flow. Arguably that might be better to keep the locality of the information but that's something you have to do even if you just want to merge all of them to a single error handler.

So I agree with the @seansfkelley here, existing language features are already good enough to handle very similar cases. I don't see any compelling reason why ?= is a better alternative without additional safety guarantees, and those are outside of the scope of the proposal unfortunately.

Edit: I have misread the handler functions incorrectly at first

Edit: the try/catch version also has the advantage of automatically bubbling the error up (also see the incomplete approach at #16)

@anacierdem
Copy link

I really want to be proven wrong here. This comes up so often that I started to believe I am missing something very obvious.

@seansfkelley
Copy link
Author

It seems like the core improvement provided is that of removing a block scope and avoiding let declarations. That is, the awkward declare-then-initialize pattern you sometimes have to do:

let result;
try {
  result = doStuff();
} catch (e) {
  handleError(e);
  return;
}

can instead avoid non-constants and some amount of scopes/nesting:

const [e, result] ?= doStuff();
if (e) {
  handleError(e);
  return;
}

Or put another way: the try block is inlined into the containing block, the catch block is left as-is, and finally has no analogue. The proposal strikes me as incomplete: of the 3 parts involved in error handling, 1 is slightly ergonomically improved, 1 is effectively untouched, and 1 is missing. With the cost of new syntax that also introduces an alternate C-style pattern for handling errors, I don't think it's the right tradeoff. I say this as someone who finds the declare-then-initialize pattern very unpleasant.

The reference to Rust in the proposal is telling; Rust's Result is a monad(-ish? I'm not an expert) with syntax-level support, but JavaScript already has a monad-ish with syntax-level support, which is Promises. True that you can't use them for synchronous work, but I don't think the juice is worth the squeeze.

Perhaps what's really missing from JavaScript is blocks-as-values. ;)

const result = try {
  yield await fetch(...);
} catch (e) {
  handleError(e);
  return;
};

@anacierdem
Copy link

anacierdem commented Aug 16, 2024

1 is effectively untouched

I think it becomes harder to combine errors for the catch case. You now only have a single option and that is checking each error individually.

1 is slightly ergonomically improved

If you factor in the possibility of skipping the error check by mistake -thus silencing it- it is not worth it just to skip a few curly braces.

Declare then initialize is avoidable if you lift the try/catch higher.

try {
  const result = doStuff();
  // use result here, no need to leave the scope to consume some intermediate value.
} catch (e) {
  // handle all errors here, or get rid of try/catch to delegate to the caller
}

Generally it is just better to have a single try/catch higher in the tree. Using it multiple times for each and every throwable is kind of an anti-pattern to me. OTOH, indeed there are some cases where you just don't care about the error and you'll need a lone try/catch, but there are already solutions to those. For example:

const resultWithDefault = await doStuff().catch(() => "default value");

or if it is sync, with a simple helper:

const resultWithDefault = maybe(() => doStuff(), "default value");

Edit: fixed sync example to defer calling the function

@rafageist
Copy link

I want to emphasize that writing less code doesn't necessarily mean writing better code. While brevity can be useful, it's important to consider that the JavaScript interpreter may end up doing more work, analyzing more code, or facing new challenges. Introducing a new operator like ?= could lead to more documentation, a steeper learning curve, and additional complexity for developers. Furthermore, many languages use try-catch, and that knowledge is reusable, reinforcing the idea that familiar patterns should be favored.

For more on this perspective, see #24.

@arthurfiorette
Copy link
Owner

@anacierdem

This is not structured correctly, see this equivalent code (error types are made up here):

Only if the functions being called explicitly tells you what classes they throw. For example, you have no way to distinguish where a TypeError comes from.

@arthurfiorette
Copy link
Owner

@anacierdem

const resultWithDefault = maybe(doStuff(), "default value");

this helper is impossible to exists, because doStuff() is evaluated before maybe().

@anacierdem
Copy link

You're right, that's my bad. Should have not call the function there. For the unknown error type case can you provide a practical example where this new feature would be useful?

@getify
Copy link

getify commented Aug 21, 2024

Just a general comment: the automatic bubbling of thrown exceptions is "nice" in one sense, but the actual experience of it is that every level either needs a try..catch, or the exception sort of hijacks the flow control. IMO the main value of this proposal is allowing code to communicate "errors" that bubble up a call-stack, but without that being an automatically flow-control-interrupting signal (or requiring the extra overhead of try..catch at each step).

In the FP world, when we deal with a type like Either (basically what this [ err, data ] tuple is), we have the ability to explicitly decide when to fork flow control to handle an error, versus just performing other tasks and returning the Either value on up the stack.

@anacierdem
Copy link

I see the potential value, but I don't think this is framed as such. My main issue with this is that -contrary to the original intention- people will see this as a way to skip error handling. If there was a way to enforce handling of the error or bubbling it up at a language level, I wouldn't mind how it "looked" much. Exposing the error in a tuple just makes it a regular value that you can throw away.

The new language feature should provide more guarantees if we want to create a monadic error handling mechanism, rather than just handing us an array.

@arthurfiorette
Copy link
Owner

people will see this as a way to skip error handling. If there was a way to enforce handling of the error or bubbling it up at a language level, I wouldn't mind how it "looked" much. Exposing the error in a tuple just makes it a regular value that you can throw away.

Absolutely not, this feature is a opt-in way to get your errors as values to treat them. If you want to ignore the error you can already do that using .catch(() => void 0) or catch {} statements. This proposal, otherwise is focused on easing the syntax to handle errors, with functions like JSON.parse being strongly used with this new proposal.

Also and obviously, alongside with ignoring the error, you will NOT have the function result, so people won't be able to ignore errors easier, but instead are now able to handle it easier and in places they wouldn't put a try/catch before.

Just count of how many times you saw someone using JSON.parse without a try/catch.

@anacierdem
Copy link

Catching the error with a catch block is very explicit in that you're using an error handling mechanism. Here, we are hiding that fact behind a "safe assignment". Getting this accepted as an error handling construct is very difficult. Destructure the array and you have a value. The fact that it's representing an error is very abstract unless everyone accepts that as a common pattern akin to return values representing errors in C.

Most of the time if I want a catch block with a small scope, I probably want a "default value". JSON.parse is probably the best example. Still, a try helper with a default value makes perfect sense.

Also and obviously, alongside with ignoring the error, you will NOT have the function result ...

How so? Maybe this is the part I'm missing. If I don't check the error, there is nothing stopping me from trying to access the value. For an occasional error I may not even realize this until it happens.

@getify
Copy link

getify commented Aug 21, 2024

If I don't check the error, there is nothing stopping me from trying to access the value

That indeed might not be automatically enforced, but... that's not supposed to be how you construct and consume such values. In other words, you don't return both a success and an error, you return one or the other. The position in the tuple here just helps you identify the value designation (error vs success).

Again to reference Either from the FP world, the value is either a Left (error) or a Right (success); it's impossible to be both at once.

How I might use something like that would be:

// was there an error?
if (result[0] != null) {
   // do some alternate or cleanup work

   // now, pass this result back up the call-stack
   // for further processing
   return result;
}
else {
   // handle result[1] success value
}

The contention I'm making is, this sort of thing, where you're checking whether the result was success or error, and performing some different operations as a result, WITHOUT the normal processing having been interrupted by an exception, and requiring a try..catch construct to pause on such interruption... that it's more preferable (flexible, less-intrusive) to have this passive [ error, success ] tuple to handle, than to rely on try..catch..thrown-exception ceremony.

@arthurfiorette
Copy link
Owner

If I don't check the error, there is nothing stopping me from trying to access the value.

How can you access the value?

Please show me an example

@rafageist
Copy link

@anacierdem

const resultWithDefault = maybe(doStuff(), "default value");

this helper is impossible to exists, because doStuff() is evaluated before maybe().

You're right that the helper mentioned doesn't work due to doStuff() being evaluated before maybe(), but the approach I suggested in issue #24 does work effectively. Your proposal, as an error-handling strategy or design pattern, is solid and not entirely new—it's a good practice. However, the real issue is that the language already allows us to achieve the same results without adding anything new. Complicating the language might be unnecessary when we can implement this pattern with existing features.

@arthurfiorette
Copy link
Owner

However, the real issue is that the language already allows us to achieve the same results without adding anything new.

List of accepted proposals that could be achieved with previous language features:

@anacierdem
Copy link

anacierdem commented Aug 22, 2024

Please show me an example

This is from the proposal:

// This suppresses the error (ignores it and doesn't re-throw it)
const [, data] ?= fn()

Another way of doing it:

const result ?= fn()
doSomethingWith(result[1])

that's not supposed to be how you construct and consume such values. In other words, you don't return both a success and an error, you return one or the other

That makes it a convention rather than a language feature IMHO. This is somewhat a contrived example but consider this:

// A well behaving function, considers result to be an Either:
const inner = () => {
   const result ?= underlyingFunction();
   if (result[0] != null) {
      // Propagate state
      return result;
   }
   else {
      // handle success
   }
}

// In a different place, someone uses this function:
const result = inner();
// I have no idea this was an Either anymore:
useValue(result[1]);

The consumer of inner is disconnected from this reality, and I believe there is no way this proposal can prevent this. It even explicitly leaves it out of scope. What if this inner function was a library? It is assumed that at every layer of execution, ?= is used.

... WITHOUT the normal processing having been interrupted by an exception, and requiring a try..catch construct to pause on such interruption ...

Even though this is indeed very attractive -and I agree there is some value- I still disagree that such a language feature should only convert your return value/error into an array. Especially when it is straightforward enough to make similar helpers.

I think this #22 (comment) explains it much better than I could. I am not sure how it can be achieved but this new operator is far away from what Rust's ? can achieve.

@arthurfiorette
Copy link
Owner

You are overthinking.

const inner = () => {
   // propagate error
   const result = underlyingFunction();

   // handle success
}

@getify
Copy link

getify commented Aug 22, 2024

IMO, consuming code should get to decide whether and how to respond to an error, not the code that may produce an error. That's why "exceptions" are considered by most to be side effects, because the "side effect" they have is interrupting the flow control of the caller, just to communicate the error up to it. It's not a small fact that being able to handle errors and values with the same level of flow-control makes code more predictable, than when errors are elevated to this shouting "Interrupt everything!" type of behavior.

Your objection to the convention over enforcement is actually another way of saying the same thing: you want the producer to control/enforce that the consumer must have known about the error. It seems to bother you that the calling code is able to cover up or dismiss errors. But that's precisely what a separation of concerns (in levels of abstraction) promises... that the concerns of the consumer and the concerns of the producer are separate, and shouldn't be tightly coupled.

@DScheglov
Copy link

@seansfkelley and @anacierdem

?= allows to write code in more readable way then we can write now.
So, it is really great thing.

And with TS having the correct typing for return type of ?= we can discriminate the error and value cases.

@arthurfiorette

If I don't check the error, there is nothing stopping me from trying to access the value.

How can you access the value?

Please show me an example

We can discriminate doesn't mean we MUST. Especially with JS.
Even with TS having strictNullChecks: false we can access the value.

So, that is your example: TS Playground

The ?= by itself doesn't enforce for correct error handling.
It provides a convenient way to write handling code.
But the error handling could be the same wrong as with try .. catch.

@DScheglov
Copy link

DScheglov commented Aug 22, 2024

@getify

minor off-topic: Many Many thanks for your Functional Light JS Book!!!
It is one of my favorite.

Your objection to the convention over enforcement is actually another way of saying the same thing: you want the producer to control/enforce that the consumer must have known about the error. It seems to bother you that the calling code is able to cover up or dismiss errors. But that's precisely what a separation of concerns (in levels of abstraction) promises... that the concerns of the consumer and the concerns of the producer are separate, and shouldn't be tightly coupled.

Using the Either (or Result like in Rust) we are enforcing the consumer to handle the Left: or explicitly, or "implicitly" reflecting the "unhandled" error in the calling function signature. Or am I wrong?
So do we violate the concern separation?

@DScheglov
Copy link

DScheglov commented Aug 22, 2024

@getify

Additionally regarding the

Your objection to the convention over enforcement is actually another way of saying the same thing: you want the producer to control/enforce that the consumer must have known about the error.

The initial intent of objection is to say that ?= must provide us with a more "robust" error handling. So, it doesn't seems to be about the producer, it is about the consumer. So, if consumer decided to use the ?= it must be forced to handle the error correctly (but what "correctly" means nobody knows)

@anacierdem , please correct me if I incorrectly got your intention.

@anacierdem
Copy link

anacierdem commented Aug 22, 2024

consuming code should get to decide whether and how to respond to an error, not the code that may produce an error …

This, I agree. I’m not opposing this idea but I also believe it is possible to bubble the error up without interrupting the control flow of the caller. I just want to make sure the developer is 100% aware about they are handling an error.

I think there are two important requirements here:

  • Bubbling of unhandled errors. This should be the default. This doesn’t have to be exception driven though. As suggested elsewhere around issues, an exception can be a “panic” in this new realm.
  • Enforcing doing something with the error otherwise. This includes discarding it but it should be explicit.

Unfortunately the current proposal doesn’t fulfill any. The first is not possible because the proposal is about the place we do the assignment and doesn't involve the caller context. The second is also problematic because the "error handling" is being delegated to a basic if statement or skipped array element, which is not something anyone would relate to error checking by default.

These facts also limits or risks the possible future extensions around incrementally adding these additional safeguards. Let's say we wanted to implement enforcement in the future, how would we define it? Would we say "there should be an if statement checking if the error value is truthy", what about if there are additional conditions?

... that's precisely what a separation of concerns (in levels of abstraction) promises... that the concerns of the consumer and the concerns of the producer are separate, and shouldn't be tightly coupled.

I don't think adding more constraints around how an error can be manipulated is coupling the producer of values/errors to the consumer. It will instead strenghten the decoupling as you can build your producer knowing the language will make sure an error value you produce is always handled gracefully.

And with TS having the correct typing for return type of ?= we can discriminate the error and value cases.

This is a js feature, we shouldn't depend on/assume anything else IMHO. Again there's someone else that can put things into words better than me: #32

I get that this is not aiming at solving the things I described and they are probably the topic of another proposal. But still, this specific proposal is not even building towards them, and that's my main objection. If we can at least have a glimpse of such a thing built atop, I will be fully onboard.

The bottomline is, for such a weak (not fulfilling my requirements and arguably hard to extend) assignment operator, I don't think we need a language level feature considering achieving the exact same thing is almost trivial (even without exceptions). If there must absolutely be a syntax for this, it is too opinionated as apparent from other discussions like #30.

I think I am going towards "agree to disagree" at this point. I definitely see the value here but the tradeoffs are not worth it to make this a feature in my view.

Edit: P.S @getify functional light js is one of my favourites as well. Learned a lot from it ❤️

@seansfkelley
Copy link
Author

seansfkelley commented Aug 22, 2024

?= allows to write code in more readable way then we can write now. So, it is really great thing.

"Readable" is subjective and should also factor in considerations like syntactic consistency. The objection raised by myself and others is that this structure is inconsistent with existing language constructs and would likely lead to a unnecessary divergence in code style and attendant developer overhead/tooling complexity for a trivial savings in keystrokes. Or, to quote someone from upthread:

I want to emphasize that writing less code doesn't necessarily mean writing better code.

Moving along...

And with TS having the correct typing for return type of ?= we can discriminate the error and value cases.

What do you mean by this? If you're referring to the type of the error value itself, that will never happen. If you're referring to distinguishing [undefined, T] from [unknown, undefined]... sure, but that's not really different from

// declare as non-nullable type
let value: T;
try { 
  value = doStuff();
} catch (e) {
  // handle, but realistically you probably don't need to let `e` escape this block
}
// compiler will make sure `value` is actually non-null before you try to use it

@DScheglov
Copy link

DScheglov commented Aug 22, 2024

@seansfkelley

?= allows to write code in more readable way then we can write now. So, it is really great thing.

"Readable" is subjective and should also factor in considerations like syntactic consistency. The objection raised by myself and others is that this structure is inconsistent with existing language constructs and would likely lead to a unnecessary divergence in code style and attendant developer overhead/tooling complexity for a trivial savings in keystrokes. Or, to quote someone from upthread:

I mean that your example definitely does not the same as ?= does.
But the next one, does

// declare as non-nullable type
let result: [{}, undefined] | [undefined, T];
try { 
  result = [undefined, doStuff()]
} catch (e) {
  result = [
   e ?? new Error("Nulish value is thrown from doStuff"), // yes it must ensure that e is not null or undefined
   undefined
  ]
}

const [error, value] = result;

The initial your example with chained promise definitelly does a different thing.
As instance if handleRequestError throws, the correspondent exception will
be caught in handleParseError, but with ?= it will be thrown from getData. And yes,
handleRequestError also can throw. Actually to wrap the exception thrown
by fetch to something like new GetDataError('FETCH_ERROR') and then throw it
could be a good error handling in this case

@seansfkelley
Copy link
Author

My example doesn't do the same thing exactly because, with thrown errors, one usually does not need to continue the main body of the function after they are caught. That's why my example says

// handle, but realistically you probably don't need to let `e` escape this block

if I wanted to shoehorn Result-style error handling into try-catch at the call site, then yes, your translation would be more accurate. But the point I was trying to make is that I don't:

The objection raised by myself and others is that this structure is inconsistent with existing language constructs and would likely lead to a unnecessary divergence in code style and attendant developer overhead/tooling complexity for a trivial savings in keystrokes.

And if I did, I would use the pattern mentioned upthread, here simplified to match the case we're talking about:

const [error, value] = maybe(() => doStuff(thing1, thing2));

While I don't generally like thunks, I don't think their attempted removal here justifies new syntax and new language semantics for everybody when a utility method like the above is so easy to write:

function maybe(thunk) {
  try {
    return [undefined, thunk()];
  } catch (e) {
    return [e, undefined];
  }
}

@DScheglov
Copy link

DScheglov commented Aug 22, 2024

@seansfkelley

// handle, but realistically you probably don't need to let e escape this block

to use the caught error could be one of the reasons to use ?=.

If we don't need error outside of the catch block, the ?= is more concise and, in this case, it means: it is more readable, because we need less time (and effort) to read the code and get what is going on.

I like your maybe ))), but don't forget to ensure that caught error is not null or undefined )

While I don't generally like thunks, I don't think their attempted removal here justifies new syntax and new language semantics for everybody when a utility method like the above is so easy to write

I'm absolutely agree that to write maybe is not a problem. I've already written the one do-try-tuple )))
But I'm also agree with @arthurfiorette that there are a lot of other cases when we got a new
syntax, having simple helper. For example ??.

@arthurfiorette
Copy link
Owner

justifies new syntax and new language semantics for everybody when a utility method like the above is so easy to write

Example: https://github.com/arthurfiorette/tinylibs/blob/476fea076cbac0e43b531be5255a06fe6ceb1f8b/packages/fast-defer/src/create-deferred.ts#L5
Accepted Proposal: https://github.com/tc39/proposal-promise-with-resolvers

@DScheglov
Copy link

@arthurfiorette

Example: https://github.com/arthurfiorette/tinylibs/blob/476fea076cbac0e43b531be5255a06fe6ceb1f8b/packages/fast-defer/src/create-deferred.ts#L5
Accepted Proposal: https://github.com/tc39/proposal-promise-with-resolvers

Actually it is not a relevant example. Promise.withResolvers doesn't introduce a new syntax. This proposal does.

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

7 participants