search

jarohen / chord

A library designed to bridge the gap between the triad of CLJ/CLJS, web-sockets and core.async.
439 stars 40 forks source link

Added support for transit on the wire. #22

Closed tgetgood closed 9 years ago

tgetgood commented 9 years ago

Thanks for the great little lib, it's saving me a lot of trouble.

I've added support for transit as the data encoding for my own purposes and thought I'd share it. From past pull requests you seem to be on the fence about how many batteries you want chord to have, and to be honest I don't know if there's a right answer.

If you think this belongs in the repo then by all means, otherwise I'd be happy enough doing what @rosejn did with chord-fressian. Just give the word.

jarohen commented 9 years ago

Hi Thomas, thanks for the PR!

As you guessed, I was very much on the fence as to whether this kind of feature should be part of Chord or not. Recently, though, I'm coming down on the side of including it in Chord - mainly because I want to make a couple of what should be internal changes to Chord, but unfortunately chord-fressian already depends on it, so would need an update as well. @rosejn, if you're reading this, would you mind me incorporating chord-fressian into the main Chord library if it came to it?

Incidentally, @lsnape has already written a chord-transit library a while back, but I'm pretty sure he wouldn't mind it being merged into Chord if I asked nicely. (He's a colleague of mine!)

I'm away from my computer at the moment, so will test and merge this over the weekend - from what I can tell though, it looks a good and concise PR, thanks! Glad you're finding Chord useful :)

James

rosejn commented 9 years ago

Hi guys, Sure no problem. Feel free to integrate chord-fressian however makes things easiest.

-Jeff

On Nov 28, 2014, at 12:32 PM, James Henderson notifications@github.com wrote:

Hi Thomas, thanks for the PR!

As you guessed, I was very much on the fence as to whether this kind of feature should be part of Chord or not. Recently, though, I'm coming down on the side of including it in Chord - mainly because I want to make a couple of what should be internal changes to Chord, but unfortunately chord-fressian already depends on it, so would need an update as well. @rosejn https://github.com/rosejn, if you're reading this, would you mind me incorporating chord-fressian into the main Chord library if it came to it?

Incidentally, @lsnape https://github.com/lsnape has already written a chord-transit https://github.com/lsnape/chord-transit library a while back, but I'm pretty sure he wouldn't mind it being merged into Chord if I asked nicely. (He's a colleague of mine!)

I'm away from my computer at the moment, so will test and merge this over the weekend - from what I can tell though, it looks a good and concise PR, thanks! Glad you're finding Chord useful :)

James

— Reply to this email directly or view it on GitHub https://github.com/james-henderson/chord/pull/22#issuecomment-64923412.

jarohen commented 9 years ago

Thanks Jeff :)

I've incorporated Thomas's PR and chord-fressian into the 0.5.0-branch branch, and used the opportunity to refactor the different formats so that they're common to both the WebSockets and the (new) AJAX support.

Unfortunately I can't deploy to Clojars at the moment (somehow I've lost access to the 'jarohen' group - am working with them at the moment to resolve), as soon as this is sorted out, I'll cut a 0.5.0 release. (You can, of course, test it in the meantime by checking out that branch, and running lein install)

Thanks both for your help :)

James

lsnape commented 9 years ago

Hey sorry late to the party on this one!

Happy for you to go ahead James, let me know if you need a hand :)

jarohen commented 9 years ago

Thanks all for your help - I've just released 0.5.0 stable, which has all of your formatting work in.

Cheers, have a good Christmas!

James