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

Counter proposal : type safe try/catch #43

Open
y-nk opened this issue Sep 11, 2024 · 13 comments
Open

Counter proposal : type safe try/catch #43

y-nk opened this issue Sep 11, 2024 · 13 comments

Comments

@y-nk
Copy link

y-nk commented Sep 11, 2024

The proposal seems copying Golang famous value, _ := blah() but I think we can do better in error handling if we copied dart/java instead.

Considering the 1st example:

async function getData() {
  const [requestError, response] ?= await fetch(
    "https://api.example.com/data"
  )

  if (requestError) {
    handleRequestError(requestError)
    return
  }

  const [parseError, json] ?= await response.json()

  if (parseError) {
    handleParseError(parseError)
    return
  }

  const [validationError, data] ?= validationSchema.parse(json)

  if (validationError) {
    handleValidationError(validationError)
    return
  }

  return data
}

I would rather leverage a new on XXX catch (err) instead which would look like this:

async function getData() {
  try {
    cons res = await fetch("https://api.example.com/data")

    return validationSchema.parse(
      await res.json()
    )
  }
  on RequestError catch (err) {
    handleRequestError(err)
  }
  on ParseError catch (err) {
    handleValidationError(err)
  }
  on ValidationError catch (err) {
    handleValidationError(err)
  }
  catch (err) {
    handleUnknownError(err)
  }
}

Advantages over your proposal:

  1. we can read happy path without "if error" interrupting every 2 lines
  2. the example provided does not provide type checking on the errors so you will end up with a if/else or a big switch case if you missed one or if the errors are not granular in the "below" layer (for example, try adding network error handling in fetch call and we'll see)
  3. it encourages people to throw (typed) Error instances (rather than returning strings and whatnot)
  4. try catch can provide an escape hatch like a global catch with the final catch, but this not really different from the proposal
  5. errors can still bubble up in the stack which is very powerful

EDIT: this addresses your concerns about nested try/catch

EDIT2: would consider also rethrow as a syntaxic sugar of throw err, that'd be nice but it's not really inside the proposal

@y-nk
Copy link
Author

y-nk commented Sep 12, 2024

@nicolo-ribaudo indeed it is! the syntax feels a bit odd to me because of those 2 reasons:

  1. the 1st when: would introduce colon to create a block (instead of curly braces)

  2. the 2nd e is X feels like competing with typescript use of is keyword and is more a assertion (it IS) rather than on which is similar to when

Alternatively, having:

try {
    cons res = await fetch("https://api.example.com/data")

    return validationSchema.parse(
      await res.json()
    )
  }
  when RequestError catch (err) {
    handleRequestError(err)
  }

...could also work, but idk, on feels a bit more fluent 🤷

@ljharb
Copy link

ljharb commented Sep 12, 2024

“on” is a word that’s associated with event listeners and observables; i don’t think that makes sense. The pattern matching proposal may add an is operator, which is what that would be based on. TS only uses it for predicate return syntax, which is in type space and thus irrelevant.

@y-nk
Copy link
Author

y-nk commented Sep 13, 2024

@ljharb i don't have strong opinion on it. when also suits, catch (err is X) also suits... since they both fit the underlying idea of having an internal branching based on err instanceof X.

the difference imho, is in where the keyword is located. with when Class catch (err), i think it reads easier if the type of err is X then catch because of how the words are placed. with catch (err is X) i personally see more an declaration than a if branching, but that may be only me.

@FrameMuse
Copy link

What about Python-like style

try {
    cons res = await fetch("https://api.example.com/data")

    return validationSchema.parse(
      await res.json()
    )
}
except RequestError (error) { }

@bruno-brant
Copy link

Of course, this violates duck typing principles as now the "type" of the error has meaning.

@y-nk
Copy link
Author

y-nk commented Dec 26, 2024

@bruno-brant there's a world beyond typescript. instanceof didn't wait 2015 to come up. the class of the error always had meaning. the fact that you can throw anything (including undefined) is a sad side effect of the language, but most api will should throw Error or sub class instances.

@ljharb
Copy link

ljharb commented Dec 26, 2024

@y-nk "duck typing" has nothing to do with typescript, and being able to throw any value is neither a side effect nor sad, it's a critical functionality that every piece of the language must always design to include.

Additionally, instanceof should never be used for builtins, since it's unreliable - it doesn't work cross-realm, and it can be a lie or be faked due to Symbol.hasInstance.

@bruno-brant
Copy link

@y-nk not sure I understand your remark, as the proposal is for JavaScript, and I've never even mentioned TS.

And the fact that you can test the prototype of an object does not mean that we should forget that JavaScript was conceived as a duck typed language.

Back to your point, you show a sample of handling errors in the fetch API. I take that is a fictional example, as I've not found any documentation out there defining that those classes will be thrown. Even if they are thrown - you might've confirmed that in code - if they aren't documented, there's nothing preventing it from changing tomorrow from ParseError to JsonError, which would break your code. So, it has always been my understanding that in Javascript testing a type of an object is a bad practice that can lead to errors.

@DScheglov
Copy link

@ljharb

You are definitely right about the issues with different realms and the possibility of faking using Symbol.hasInstance. However, we already have a large codebase that throws error instances and expects exceptions to be identified using instanceof.

Only a small number of libraries provide error-detecting predicates, and an even smaller number identify their own exceptions without relying on instanceof under the hood.

Therefore, using instanceof with exceptions is expected.

Perhaps only Node.js is the exception, as it provides a wide set of error codes for different cases. But in my own practice I'n not identifying them at all, just catching in the error boundary and reporting to sentry.

But for error codes, it's better to use a Result<T, E> object instead of throwing exceptions because it can be more strongly typed. Even for JavaScript projects, this is valuable because .d.ts files improve the developer experience with better autocompletion.

@ljharb
Copy link

ljharb commented Dec 26, 2024

“we already have a large codebase doing a bad thing” is not a persuasive argument to me to cargocult something into the language.

In TS, a tuple can be named just like a record/interface/object, and we should ne designing things based on what makes the most sense for JavaScript, not for a totally different language.

@DScheglov
Copy link

“we already have a large codebase doing a bad thing” is not a persuasive argument to
me to cargocult something into the language.

@ljharb , I'm not sure I got your idea correctly. But when I talked about a large codebase,
I just meant that instanceof is currently widely used, so it's not a good idea
to simply ignore it or claim it as "a bad practice", even though there are some
edge-case issues.

About cargocult...

EcmaScript borrows a lot from other languages (classes, generators, even arrow
functions), and we don't call that cargocult. They might say this entire proposal
could be considered as a cargocult of Go's manual error propagation...

The Result type is not a replacement for exceptions; it just works better
with error codes than exceptions do. But in Go, the Result approach is exactly
a replacement for exceptions, because the panic! macro isn't convenient for
discriminated error handling. So, it's not a cargocult at all.

@ljharb
Copy link

ljharb commented Dec 26, 2024

Wide usage never precludes something being a bad practice, and the issues with instanceof aren’t edge cases.

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

6 participants