#327 notifyAsync

[subzero10]

Status

READY

Description

Related Issue: #327

I came up with a simple implementation of notify that returns a promise (notifyAsync).
It all became simple because now we always call the afterNotify hook, so I used it to know when to resolve/reject the promise.
I decided to go for this because of our recent work on AWS Lambda (see here and here). The fire-and-forget approach does not work well with Lambda functions.

Let me know what you think.

Note: The target branch is aws_lambda_async_#677

Todos

• Tests
• Documentation
• Changelog Entry (unreleased)

[shalvah]

Great! Love the test coverage. I do wonder, though—isn't there a global version of afterNotify? From the docs:

Honeybadger.afterNotify((err, notice) => {
  if (err) { return console.log(`Honeybadger notification failed: ${err}`); }
  console.log(`Honeybadger notice: https://app.honeybadger.io/notice/${notice.id}`);
});

I didn't see that covered here.

[subzero10]

Great! Love the test coverage. I do wonder, though—isn't there a global version of afterNotify? From the docs:

Honeybadger.afterNotify((err, notice) => {
  if (err) { return console.log(`Honeybadger notification failed: ${err}`); }
  console.log(`Honeybadger notice: https://app.honeybadger.io/notice/${notice.id}`);
});

I didn't see that covered here.

Thanks!
I wanted this afterNotify hook to be the last to be called.

And here's what I just realized:

• The afterNotify hook that you specify with Honeybadger.notify() is actually called first.

// util.ts
export function runAfterNotifyHandlers(notice: Notice | null, handlers: AfterNotifyHandler[], error?: Error): boolean {
  if (notice && notice.afterNotify) {
    handlers.unshift(notice.afterNotify)
  }

  for (let i = 0, len = handlers.length; i < len; i++) {
    handlers[i](error, notice)
  }
  return true
}

I think I can just change the code to:

return new Promise((resolve, reject) => {
  this.afterNotify((err) => {
          if (err) {
            return reject(err)
          }
          resolve()
  })
  this.notify(noticeable, name, extra)
})

and skip all the block of code of objectToOverride. Then add a test to check that this hook is indeed called last.

[shalvah]

Okay, that sounds cool.

Glad we're adding this. Hopefully we can even get rid of afterNotify in the future. It's a bad pattern, basically reimplementing continuations, which Node.js has supported since forever (callbacks, promises, events).

[subzero10]

@joshuap Waiting for your feedback on this. We should also mention notifyAsync in the documentation, but I'm not sure how to. notifyAsync is a must for lambda functions, but optional server or browser.

[subzero10]

Glad we're adding this. Hopefully we can even get rid of afterNotify in the future. It's a bad pattern, basically reimplementing continuations, which Node.js has supported since forever (callbacks, promises, events).

Sounds like another topic for discussion!

[shalvah]

On second thought, since notify might be called by code the end user, I guess it's not a bad pattern at all.

[joshuap]

Nice, I like the approach! See my comment below.