ovhemert / pino-applicationinsights

A transport for pino that sends messages to Azure Application Insights
MIT License
10 stars 12 forks source link

Why is createWriteStream async? #56

Open devinrhode2 opened 2 years ago

devinrhode2 commented 2 years ago

next-logger expects to resolve a logger function synchronously, without needing to await anything. (https://github.com/atkinchris/next-logger/issues/12)

I see that createWriteStream does not actually use the await keyword: https://github.com/ovhemert/pino-applicationinsights/blame/d85e7426d2f7fe6cb45557631bac6eba26cd2b41/src/index.js#L7

And, searching code from the initial commit for await it does appear to not be necessary: https://github.com/ovhemert/pino-applicationinsights/commit/1e02b7289e6e5eb80c7187f3d19e3b6559f51cab

I assume it's there to reserve the capability to use await should it be necessary in the future.

Could I create a PR which creates an alias, createWriteStreamSync which simply doesn't have the async keyword? Code will end up looking something like:

function createWriteStreamBase() {
  // ... 
}

// This forces callers to use `await` which in turn will allow us to use `await` should we need to in the future (without causing any breaking changes)
export const createWriteStream = async (...args) => createWriteStreamBase(...args)

// Should `createWriteStream` need to resolve asynchronously in the future, this method may be removed.
export const createWriteStreamSync = createWriteStreamBase

Although breaking changes of course always keep code cleaner, this library is small, doesn't have a ton of users, so I think breaking changes may be preferable in this scenario.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.