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

Why is createWriteStream async? #56

Open
devinrhode2 opened this issue Mar 4, 2022 · 1 comment
Open

Why is createWriteStream async? #56

devinrhode2 opened this issue Mar 4, 2022 · 1 comment
Labels
wontfix This will not be worked on

Comments

@devinrhode2
Copy link

devinrhode2 commented Mar 4, 2022

next-logger expects to resolve a logger function synchronously, without needing to await anything. (sainsburys-tech/next-logger#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: 1e02b72

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
Copy link

stale bot commented Aug 13, 2022

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.

@stale stale bot added the wontfix This will not be worked on label Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant