haxetink / tink_streams

Streams from the future. With lasers, of course ... whoaaaaa!!!!
The Unlicense
12 stars 9 forks source link

Revamped the API and implementation of Accumulator #11

Closed kevinresol closed 7 years ago

kevinresol commented 7 years ago

I think that the main objective of an Accumulator is for manually yielding results, and it does the job. But currently the implementation has a downside that (if I understand correctly) it holds the stream "root", meaning that for long-running streams like a tcp connection, the "whole history" will be held in memory until the accumulator reference is let go. Here I suggest implementing Accumulator over a Signal(Trigger), so that the user only holds the reference to the trigger, thus allowing the stream history to be garbage collected.

As a result, there will be both an API and behavior change:

before:

var a = new Accumulator();
a.yield(Data(1));
a.yield(Data(2));
a.yield(End);
a.forEach(...); // gets 1, 2

now:

var t = Accumulator.trigger();
var a1 = t.asStream();
t.trigger(Data(1));
var a2 = t.asStream(); // only data that is triggered after creating the stream will be available to this stream instance
t.trigger(Data(2));
t.trigger(End);
a1.forEach(...); // gets 1, 2
a2.forEach(...); // gets 2
back2dos commented 7 years ago

I'm fine with it. My suggestion would be to call asStream differently, as the current name implies a sort of "cast" or something, when really it creates a new stream - as opposed to SignalTrigger.asSignal that always gives you back the same signal. But I'll let you be the judge of that ;)

kevinresol commented 7 years ago

If you don't mind letting go Accumulator I can simply remove it and promote using SignalStream

back2dos commented 7 years ago

Sounds like a plan.

kevinresol commented 7 years ago

Yay!