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

Ensure error classes are exported #1254

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

julienvincent
Copy link
Contributor

Fixes #982

The KafkaJS error classes currently aren't exported which makes it hard to distinguish between application errors and kafka errors, especially while developing utils/libraries on top of KafkaJS.

This is especially confusing as the TS types included do export the error classes meaning these errors are only discovered at runtime.

@Nevon
Copy link
Collaborator

Nevon commented Dec 17, 2021

Damn. I think it was a mistake to export the error classes from TS to begin with. We should probably have exported error interfaces instead, because there's no situation in which you as a consumer of KafkaJS should be instantiating those errors yourself. You just need to know what fields are publicly available on the error instances that we throw, and do figure out which error you received in case of a union, which we could do by exposing a type field on the error interface instead.

But unfortunately this was done in 1.15.0, so it's a public API now :(

@Nevon Nevon merged commit 38e0750 into tulios:master Dec 17, 2021
@julienvincent julienvincent deleted the fix/error-exports branch December 17, 2021 12:46
@julienvincent
Copy link
Contributor Author

@Nevon

because there's no situation in which you as a consumer of KafkaJS should be instantiating those errors yourself

In our case It's not that we want to instantiate the error, but more that we want to do instanceof checks on it. As an example: We have built an eachBatch util which adds some additional logic like retries, tracing, metrics, and error reporting - but we want to fall through when a KafkaJS error is encountered (which might be encountered when calling methods on a batch like heartbeat)

This looks something like:

const eachBatchUtil = (handler: kafkajs.EachBatchHandler): kafkajs.EachBatchHandler => {
  return async (payload) => {
    try {
      await handler(payload)
    } catch (err) {
      if (err instanceof kafkajs.KafkaJSError) {
        throw err;
      }

      // .. attempt retries
    }
  }
}

consumer.run({
  eachBatch: eachBatchUtil(async (payload) => { ... })
})

@Nevon
Copy link
Collaborator

Nevon commented Dec 17, 2021

Yeah, that's why I mentioned:

figure out which error you received in case of a union, which we could do by exposing a type field on the error interface instead.

We don't even do instanceof comparisons inside KafkaJS itself. Instead, each error class has a static type property that we use to determine which type of error we've received.

@julienvincent
Copy link
Contributor Author

Oh, gotcha. :)

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

Successfully merging this pull request may close these issues.

Error class are not exported
2 participants