Use promises and async/await instead of callbacks where applicable

[Mr0grog]

Promises and the async/await keywords are supported in all versions of Node.js this package is compatible with, and it would probably be good to use them instead of callbacks (e.g. in BufferedMetricsLogger.flush(). We can also support both.

[Mr0grog]

The right approach here probably depends on the question I posed in #100 — do we want to do a non-breaking 0.10.2 release (in which case I should implement this in a way that supports both promise-based and callback-based usage), or make the next release 0.11.0 and include breaking changes (in which case I do this with nicer async/await syntax and drop callback support entirely)?

@ErikBoesen any feedback on that?

[ErikBoesen]

My inclination is that we should support both syntaxes.

[Mr0grog]

Just to make sure I’m following, are you saying:

1. Support both callbacks and promises for v0.10.2, then drop callbacks and only have promises for v0.11.0+, or
2. Support both callbacks and promises for the long term?

I might not have said this clearly before, but I think (1) is fine, but am not a fan of (2). Specifically, I think the value of this issue (use promises and async/await) as well as #83 (class syntax) as #85 (update linting tools) are in making the code easier to maintain because they make it harder to make mistakes (otherwise these issues are all just yak-shaving, and we should close them instead of doing them!).

Supporting both callbacks and promises is relatively complicated: you have to not return a promise if someone passes in callbacks, because returning a promise that rejects can crash someone’s program if they don’t handle the rejection, which they will not have done if they passed in a callback. It makes the code more error-prone, rather than less. Given the maintenance status of this package, I think simpler-to-maintain code is more valuable than flexible-but-has-edge-cases code. (I think async/await is simplest to maintain and least error prone, while sticking to callbacks is still less error prone than doing both.)

[ErikBoesen]

That is a fair argument; seems like it would be best to fully replace them and do a breaking release. (As I understand it, isn't the first component of the semantic version for breaking changes, so this would be v1.0.0?)

[Mr0grog]

(As I understand it, isn't the first component of the semantic version for breaking changes, so this would be v1.0.0?)

In semver, the rules are different before v1 vs. after:

• Before v1, every minor release is considered breaking (so v0.10.x → v0.11.0 is breaking, but v0.10.1 → v0.10.2 is not).
• After v1, and only major releases are breaking (so v1.0.x → v2.0.0 is breaking, but v1.0.x → v1.1.0 is not).

Since we are at v0.10.1, you can “safely” do a breaking change in v0.11.0. (Or: you should only go to v0.11.0 if you have breaking changes. If you don’t you should go to v0.10.2.)

[ErikBoesen]

Gotcha, sounds good!

[Mr0grog]

OK, so no v0.10.2; we’re just going straight to v0.11.0 as the next release?