fix: Emit connectFailed on connection failure.
#223
Conversation
fix #222 BREAKING CHANGE: We will no longer emit a `disconnect` event on an initial connection failure - instead we now emit `connectFailed` on each connection failure, and only emit `disconnect` when we transition from connected to disconnected.
src/AmqpConnectionManager.ts
Outdated
| * successful, failed, or in-progress.. | ||
| */ | ||
| get connectionAttempts(): number { | ||
| return this._connectionAttempts; |
There was a problem hiding this comment.
Should we maybe rename it to pendingConnectionAttempts etc. and reset it on connect? Not sure ever increasing counter has sense?
There was a problem hiding this comment.
pendingConnectionAttempts makes it sound like it should either be 1 or 0 (we're trying to connect or we're not).
connectionAttemptsSinceLastConnect? That's a bit wordy.
There was a problem hiding this comment.
I'll take the connection attempt count out for now - you can always track it yourself from the event if you really need it, and then we can get this merged.
There was a problem hiding this comment.
That is perfectly fine for me, actually that's how I was doing it till now, so just need to change the listened event. Thank you
|
Ty a lot for the PR, single comment. Othewise, lgtm. |
BREAKING CHANGE: No longer running unit tests on node 10 and 12, although this package may continue to work on these.
|
🎉 This PR is included in version 4.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #222.
This makes it so instead of emitting a
disconnectevent the first time we fail to connect, AmqpConnectionManager now emits aconnectFailed. AmqpConnectionManager emitsconnectFailedon every connection failure, and emitsdisconnectonly when we transition from connected to disconnected.BREAKING CHANGE: We will no longer emit a
disconnectevent on aninitial connection failure - instead we now emit
connectFailedon eachconnection failure, and only emit
disconnectwhen we transition fromconnected to disconnected.