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

feat: allow async beforeNotify functions to modify the notice object #984

Merged
merged 17 commits into from
Jul 27, 2023

Conversation

KonnorRogers
Copy link
Collaborator

@KonnorRogers KonnorRogers commented Dec 22, 2022

Status

READY

Description

Fixes #981.
Allow async beforeNotify handlers to modify the notice object before sending to Honeybadger.

Todos

  • async beforeNotifyHandler
  • async afterNotifyHandler
  • Tests (fix integration tests)
  • Documentation

packages/core/src/util.ts Show resolved Hide resolved
packages/core/src/util.ts Show resolved Hide resolved
packages/core/test/client.test.ts Outdated Show resolved Hide resolved
@subzero10 subzero10 added the js @honeybadger-io/js label Jan 6, 2023
Copy link
Contributor

@BethanyBerkowitz BethanyBerkowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not trying to gum up progress here, but I don't feel great about how the return value of notify() is working in this PR.. let's discuss further?

packages/core/src/client.ts Show resolved Hide resolved
packages/core/src/util.ts Show resolved Hide resolved
packages/core/src/client.ts Show resolved Hide resolved
packages/core/src/client.ts Show resolved Hide resolved
packages/core/src/client.ts Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/util.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@BethanyBerkowitz BethanyBerkowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation helps, thanks. I still think it's not ideal, but I understand wanting to keep backwards-compatibility!

@subzero10
Copy link
Member

subzero10 commented Jul 27, 2023

I will go ahead and merge this to get this feature out, but here's what I'm thinking:

  • we could ditch the return value of the notify() function entirely
  • knowing that one can get this information by setting an afterNotify handler (if a notice is not going to be sent, an afterNotify handler will be called with the notice and an error)
  • this should probably be a major version bump

What do you think?

@subzero10 subzero10 merged commit bcb2b92 into master Jul 27, 2023
@subzero10 subzero10 deleted the konnorrogers/allow-async-handlers branch July 27, 2023 15:44
@joshuap
Copy link
Member

joshuap commented Jul 31, 2023

  • we could ditch the return value entirely

How would a user programmatically stop a notice from being sent?

@subzero10
Copy link
Member

  • we could ditch the return value entirely

How would a user programmatically stop a notice from being sent?

I am referring to the return value of the notify() function, not the beforeNotify handlers. Does this make sense?
(I edited my comment above to make it more clear)

@joshuap
Copy link
Member

joshuap commented Aug 7, 2023

  • we could ditch the return value entirely

How would a user programmatically stop a notice from being sent?

I am referring to the return value of the notify() function, not the beforeNotify handlers. Does this make sense? (I edited my comment above to make it more clear)

Gotcha—yeah, I think that makes sense.

@subzero10
Copy link
Member

Gotcha—yeah, I think that makes sense.

Done! See #1162.
cc @BethanyBerkowitz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
js @honeybadger-io/js
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to have async beforeNotify handler?
4 participants