Closed SimenB closed 6 years ago
Overall the change is a good one 👍 . I am concerned that the change in the message output of the error may break peoples existing apps if they are looking at the current error message to determine error handling.
See my in code comments for a couple house cleaning issues.
the change in the message output of the error may break peoples existing
I agree it's a breaking change.
I'll fix the points you raised as soon as I get to work!
@awolden Ping 😄
PR looks good :+1:, but I am hesitant to perform a major version bump for such a minor change. We will sit on this PR a bit and try to roll in with more changes.
Any other planned breaking changes? If not, I suggest just merging and releasing this. A clear changelog would mitigate the churn, I think. And a major is just a number :smile:
Hey SimenB,
Sorry for the delay. I don't want to push out a breaking change to aid in logging that can be accomplished pretty easily by the end consumer of the library. For instance you could do:
const brake = new Brakes();
brake.request('foo')
.catch(err => {
console.error(`[Breaker: ${brake.name}] ${err.message}`);
});
I would like it if Brakes handled this automatically for the user, but I also want to avoid version churn. While it is just a number, I don't want to increment a major version just for a change to aid in logging.
I will keep this PR open until it can be packaged with other changes. You don't have to worry about rebasing, I can take care of that when it is merged.
Thanks for all your contributions @SimenB!
@awolden, yeah, that's what we do now 😄
@awolden it's been over a year, could you merge and release this?
Yes, let's merge this. Can you fix the conflict on your end?
Done!
Woo! 🎉 Looking forward to release
Not sure if you want this, but we find it practical when debugging and tracing through logs.