Reconnect semantics, acks&nacks, 'cleaner' disconnects - Githubissues

easternbloc / node-stomp-client

A STOMP client for Node.js

Other

93 stars 47 forks source link

Reconnect semantics, acks&nacks, 'cleaner' disconnects #34

Closed storkme closed 9 years ago

storkme commented 9 years ago

hi,

I've made this pull request mostly to address #33 and #19, but also to add in support for ACKS/NACKS, and alter the semantics of the 'disconnect' handling to better fit my use case (in my mind the remote server disconnecting us is an error, and should attempt to reconnect).

Two tests are failing in this PR, but honestly I can't see the value in these two tests anyway:

'check the ERROR callback fires when we receive and error frame when disconnecting'
'check the ERROR callback is removed from the client on disconnect' I would be inclined to remove these two tests as to me they seem a little contrived, but perhaps there's something I'm missing here?

easternbloc commented 9 years ago

Hi @storkme can you rebase out the merge commit to master please?

easternbloc commented 9 years ago

This one: eff6061

easternbloc commented 9 years ago

The rest of it looks great by the way.

storkme commented 9 years ago

hi @easternbloc, sorry but I didn't quite follow - you want me to remove the merge commit?

I'm not all that experienced with git, could you give me some pointers?

Thanks.

easternbloc commented 9 years ago

you should be able to rebase it out of your branch. Something like

git rebase -i HEAD~6 then when it brings up the interactive rebaser just delete that last merge commit. It's just so the commit history is clean.

easternbloc commented 9 years ago

then you'll need to force push the branch as you've rewrote history so you'll need to --force the branch when you push it!

easternbloc commented 9 years ago

hope that helps.

storkme commented 9 years ago

I think I've managed it by doing a hard reset to the appropriate point, does it look better now?

easternbloc commented 9 years ago

Cool thanks. I'm just gonna check through those two failing tests when I have some time and check what value they add.

gknapp commented 9 years ago

Any idea when this PR will be merged? Have you had a chance to review the failing tests?

easternbloc commented 9 years ago

Had a look. Pretty sure @storkme is correct. Those tests can be removed. @storkme any chance you can remove them in this PR?

easternbloc commented 9 years ago

thanks @storkme this is now merged and published in 0.6 on npm. Cheers

storkme commented 9 years ago

Apologies for not removing those tests, have been too busy recently :-1:

many thanks for the merge @easternbloc :+1: