Closed mAAdhaTTah closed 6 years ago
Also, feel free to bikeshed the naming here, withCollector
vs collectable
vs. whateverYouWant
. We can all rebikeshed Aggregator
as well, although I think that API is fine as-is.
If we're going to go with combine
conceptually, we should provide the merged stream as well as access to the dependent streams, so something like this:
combine: (inst$, { onClick$, onType$ }) => onType$.sampledBy(onClick$);
We should then provide a helper function (withoutBy
?) that you can use with #thru
to get back a new stream with a provided stream removed:
combine: (inst$, { onClick$ }) => inst$.thru(withoutBy(onClick$));
would return a merged stream that contains onType$
alone.
This allows developers to say "I have a stream of 6 types of actions, and I want to interact with 2 of them and pass the rest on", they have a means of getting back a stream that to use in that way.
We should pull all of the Kefir helpers into a separate library as well (in 0.12.0).
Doing this as a high-order component has a couple nice things about it:
combine
, which should also be called with the component's props.Collector
.Collector
/ Aggregator
outright.Still considering new names as well, and deprecating both Collector & Aggregator for these new components. Something stream-y / brook-y would be more this lib's style.
We're going with toJunction
(Collector) & RootJunction
(Aggregator). A Junction is a point where two rivers come together. We also considered Confluence, but it's Atlassian's product & a bit more esoteric.
Thank you WeAllJS Slack for helping with the naming!
Currently, the
Collector
, responsible for pulling together all of the streams, currently walks to children it's passed, convertonX
callbacks to streaming callbacks. The API currently looks like this:The issue here is twofold:
button
, from a React perspective, looks the same. This is both good, because it simplifies the transition, but not so good if it produces unexpected bugs. "Doing it wrong" should be clearer.onX
callbacks can no longer be type-checked, because React's type definitions expect a function ofSyntheticEvent => T
but it's nowObservable<SyntheticEvent> => Observable<T>
. I really want to be able to fully typecheck abrookjs
app, so solving this is required.Instead, I'm proposing an API that looks like this:
This makes it explicit that you need to wrap your event listener with the
collect
function, which is responsible for wrapping your function and providing a callback to React. In addition to this explicitness,collect
can be typechecked to ensure they match.The downside in the addition of some boilerplate: both the function children as well as the
collect
callback.Alternative: HOC
Instead of wrapping in a component with a render children, we could make this a HOC that gets configured with the callbacks it needs. So an API like this:
We could flatten this configuration object if we wanted:
But I'm suggesting the
events
key because it would allow us to include acombiner
key as well, which would allow us to merge events however we'd like:And now the component will only event a typing event when a button is clicked.
Collector
current haspreplug
, which would allow you to do something similar, but it's a bit more cumbersome because we can't provide named streams to thepreplug
callback.The counterpoint is it's not clear how to handle the "modify streams by context" use case. In the above example, the resulting
Button
only emits aCLICK
action. In the current implementation, we can modify the values emitted thus:This, when embedded in a larger component, allows leaf components to emit generic actions while the container components scope those actions based on their context.
The render children version can continue to function in this manner without issue; the HOC will likely need an API change. I'm envisioning the returned component itself would expose
preplug
: