search

jarohen / chord

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

transducers on write-ch receive formatted value #59

Open AlexHill opened 4 years ago

AlexHill commented 4 years ago

I want to apply a transformation to everything I send up my websocket, so I thought I could pass a channel with a mapping transducer as write-ch.

This doesn't work as I'd hoped because my transducer sees messages after formatting, not before. So I have to thaw, apply the transformation, and freeze again in my transducer.

Ideally, I think the formatting would occur as the last thing before writing to and the first thing after reading from the websocket connection - so read-from-ws! and write-from-ws! would take a formatter, and apply thaw and freeze respectively, rather than involving core.async at all.

That way the user-supplied channels will always be dealing with unformatted values, which I suspect will be what people want/expect almost all the time, and we lose a bit of core.async overhead inside of chord. This would break backwards compatibility in that the change could break anyone passing a channel with a transducer as write-ch. However I suspect most uses of read-ch and write-ch are to apply buffering as all mine were until today :)

Happy to make this pull request if you like the idea!

jarohen commented 4 years ago

Hey Alex, thanks for getting in touch :)

Yep, I agree - would certainly be useful to be able to apply a transducer before formatting, although I'd like to try avoid a breaking change if possible - you can bet that it will break someone's workflow!

Thinking out loud:

What do you think?

Cheers,

James

AlexHill commented 4 years ago

Ok yeah, let’s rule that breaking change out.

A simple solution would be to deprecate those keywords and introduce :read-chan and :write-chan with the desired behaviour.

Alternatively, one could pass [buf-or-n xform] for read and write, mirroring the signature of core.async’s chan.

Then the transducer for the read channel could be (comp (map thaw) xform) and for the write channel (comp xform (map freeze)) and we get the desired effect.

Is it Chord's responsibility at all? what would a user have to do if Chord didn't support it? (maybe the answer to this will influence what the Chord API change should be?)

In another situation where a library gave me a channel that I wanted to apply transformations to, I would make my own channel with a transducer, pipe to/from the library's channel and pass mine around instead. But chord returns a non-standard bidirectional channel and it’s harder to pass one of those around with my transformations built-in - for that reason I think it’s nice for chord to provide this.

Thanks for your feedback!

Alex

On Thu, 24 Oct 2019 at 5:04 pm, James Henderson notifications@github.com wrote:

Hey Alex, thanks for getting in touch :)

Yep, I agree - would certainly be useful to be able to apply a transducer before formatting, although I'd like to try avoid a breaking change if possible - you can bet that it will break someone's workflow!

Thinking out loud:

  • I wonder whether we could introduce an additional pre-freeze/post-thaw channel?
  • Maybe we could allow the user to supply transducers?
  • Is it Chord's responsibility at all? what would a user have to do if Chord didn't support it? (maybe the answer to this will influence what the Chord API change should be?)

What do you think?

Cheers,

James

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jarohen/chord/issues/59?email_source=notifications&email_token=AAHW6GZMFUSIIHHQKHUP4ETQQFQKNA5CNFSM4JEQMNJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECEJJ7Y#issuecomment-545821951, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHW6G6EEGZIY74VO6FJIX3QQFQKNANCNFSM4JEQMNJQ .

jarohen commented 4 years ago

Cheers - all good suggestions.

On balance I prefer the new :read-chan/:write-chan option - it gives the user the option to create the channel however they'd like (they may even have one in hand already) and also insulates us against future changes to how people might be able to create core.async channels. Would it be something you'd have time to create a PR for?

James

AlexHill commented 4 years ago

Yep, I can make a PR.

So to summarise: I’ll add :read-chan and :write-chan options. These will be mutually exclusive with :read-ch and :write-ch - chord will throw if they’re used together.

The existing behaviour will be preserved when the existing options are used.

I’ll update the docs deprecating the existing options and describing the behaviour of the new ones.

jarohen commented 4 years ago

Great, cheers :smile: