search

lahsivjar / react-stomp

React websocket component for STOMP protocol over SockJs
https://www.npmjs.com/package/react-stomp
MIT License
134 stars 41 forks source link

Fix for changing topics #179

Closed LukasMazl closed 3 years ago

LukasMazl commented 3 years ago

Hello Lahsivjar! :)

I have made this pull request, because i would like to fix one bug, which i found out during working with ,,react-stomp".

This pull request is fixing bug #150. Main reason why this happens is that react-stomp during calling method UNSAFE_componentWillReceiveProps make subscription with XY destination topic and id: sub-0 then is called _unsubscribe. What happens when _unsubscribe is called on the stompClient side? StompClient will send unsubscribe request with id: sub-0. That means the component unsubscribes older topic and the newest topic, because they have same sub-id. After that when message came from backend side, stompClient will write ,,unhandled message error" to console log (when debug is allowed).

I have changed order of calling subscribe and unsubscribe methods, because that makes more sense to me. Also i have added unique generation of sub-id. This bug is also described here https://stackoverflow.com/questions/60318443/why-all-topics-are-getting-same-id-when-subscribeheaders-is-not-passed-in-props.

Have a nice day!

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 606

Totals Coverage Status
Change from base Build 604: 0.1%
Covered Lines: 83
Relevant Lines: 84
💛 - Coveralls
lahsivjar commented 3 years ago

Thank you for creating a PR, I have taken a look into the issue and the StackOverflow post (both of them are very detailed). It seems the root of the issue is that the object this.props.subscribeHeaders is getting passed directly to stompClient and it is altering it by adding the id key value pair. Since in JavaScript, an Object is passed by reference thus when the id is added to the subscribeHeaders by StompJs it changes the original subscribeHeaders. This is the reason we see that when the second topic is subscribed the subscribeHeader is already populated.

The fix for this would be create a copy of subscribeHeader and pass the copy to subscribe method.

lahsivjar commented 3 years ago

https://github.com/lahsivjar/react-stomp/tree/fix-subscribe-headers

I have pushed a fix in the above branch, can you check if it works as expected?

LukasMazl commented 3 years ago

That works perfectly!! :) Thank you!! I gonna close this useless PR :D

lahsivjar commented 3 years ago

Released the fix in version 5.1.0. Thanks, @LukasMazl for the detailed analysis that helped to diagnose the issue.