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 there a Promisify function exposed #352

Closed
lholmquist opened this issue Jul 9, 2019 · 1 comment · Fixed by #354
Closed

Why is there a Promisify function exposed #352

lholmquist opened this issue Jul 9, 2019 · 1 comment · Fixed by #354

Comments

@lholmquist
Copy link
Member

I was looking in the code and i saw that there is a Promisify function attached to the factory object, https://github.com/nodeshift/opossum/blob/master/index.js#L85

I did a search and it isn't actually being used. Was this used at some point or was it just for convenience for someone using this library?

If we aren't using it, perhaps we should remove it?

If we removed it, it would be a semver-major change

@lance thoughts? do you remember why it was there?

@lholmquist
Copy link
Member Author

So it looks like we can remove this, as it was added before Node had its own Promisify utility.

The only concern would be for the browser implementation, but a user could either write their own, or use an existing library that did this.

lholmquist added a commit to lholmquist/opossum that referenced this issue Jul 12, 2019
BREAKING CHANGE: Remove the Promisify function from the CircuitBreaker factory

* Node has its own built-in promisify function that can be used instead.

fixes nodeshift#352
lance pushed a commit that referenced this issue Jul 12, 2019
BREAKING CHANGE: Remove the Promisify function from the CircuitBreaker factory

* Node has its own built-in promisify function that can be used instead.

fixes #352
lance pushed a commit that referenced this issue Aug 8, 2019
BREAKING CHANGE: Remove the Promisify function from the CircuitBreaker factory

* Node has its own built-in promisify function that can be used instead.

fixes #352
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant