davinkevin / AngularStompDK

Angular service to communicate to Stomp-Websocket
http://davinkevin.github.io/AngularStompDK/
Apache License 2.0
36 stars 12 forks source link

Added unsubscribe support for topics #37

Closed brianbruff closed 8 years ago

brianbruff commented 8 years ago

Hi Kevin,

Made a slight change to support topics and unsubscriptions

Can you review and let me know if it's ok?

Btw: Nice repo, learned a few tings I'd not experienced before.

I'm going to be adding support for multiple Websocket/stomp servers. Is this something you would be interested in for this project? If so have you any strong view on the approach?

Thanks Brian.

davinkevin commented 8 years ago

First thing, your PR will always be welcomed and I'm glad you learn things from my repo, it's something I like to hear 😄

For your modification, I've reviewed it, but I would like you to integrate some modification in the README to let people know what they can do thanks to your modification and of course modification on tests.

After this, I know I haven't referenced any "contributing rules", but I would appreciate if your commit follow the "conventional-changelog" format.

If you need help, I could help you on the gitter chat-room.

PS : Sorry for the failing build, it's linked to the JSPM token not accessible in PR build.

For other improvement, I let you open an issue and discuss with you there (and maybe with other users of this lib).

Again, thanks for your contribution !

brianbruff commented 8 years ago

Hi David,

No problem, I'll update the read me and have a look at the conventional-changelog

Yes was great to learn a few different tools, hadn't used EM6 or JSPM, live mostly in a Typescript world, so was nice to see how the other half live ;-)

will turn this around tomorrow.

kr, Brian.

davinkevin commented 8 years ago

I will maybe migrate it in TypeScript, I wait JSPM to deliver the last 0.17 and I will migrate code and package manager...

PS : My firstname is Kevin, not David... I choose my username in format lastname_firstname, and everyone are confused about this 😉

PPS : This modification you made, it's to enhance the lib or to prevent a bug ? I read your modification, and I doesn't figure it, and I am not able to launch the read all the code to see it...

brianbruff commented 8 years ago

No problem Kevin

It was just a bug, when my team tried to unsubscribe from a topic it was failing. The .topic value was set on the connection and not the .queue The existing code was just checking .queue so I changed it to check both.

Probably no need to migrate it to typescript it works well as it is :-)

Sorry for short comment I'm on my phone now. Cheers Brian

davinkevin commented 8 years ago

So, maybe checking both is useless if the queue is always undefined here.

I surely made a mistake when I wrote this code... a type error, the TypeScript would have told me this 😉 And of course, I don't have the sufficient test to verify this...

Thanks for your help 😉

brianbruff commented 8 years ago

Hi Kevin,

Yes typescript is fantastic, I sold internally a year ago in my company as : "my heart says we use javascript and my head says typescript" Now my heart also says typescript, I'm sold on static typing

i'm not sure, I didn't check how it behaves if i subscribe to a queue (shame on me i should have). We're using topics exclusively in our Application.

I expected it would have been .queue if i connected to a queue and .topic if i connected to a topic, your formal parameter was called topic so I did wonder a little...

tell you what I'll test it tomorrow with both queues and topics add some tests etc I'll send a proper pull request when I've finished, I've only 2 hours each day on the project that used it.

cheers, Brian.

davinkevin commented 8 years ago

Close by commit dfe524559c334bb104ff1786d044004eb8759c45

I've removed element linked to topic to just have name 'queue'.