Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stream.observe() #2713

Closed
soulofmischief opened this issue Dec 12, 2021 · 7 comments
Closed

Add stream.observe() #2713

soulofmischief opened this issue Dec 12, 2021 · 7 comments
Assignees
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@soulofmischief
Copy link
Contributor

soulofmischief commented Dec 12, 2021

Description

Add stream.observe to allow an idiomatic way to observe potentially async parent streams.

Implementation

stream.observe = function( ...streams ) {
  return stream.merge( streams ).map( streams => {
    Promise.all( streams ).then(() => m.redraw())
    return true
  })
}

Usage

const 
  // Create async stream.
  promise = new Promise( resolve => resolve( true ),
  asyncStream = stream( promise ),
  // Observe changes.
  observer = stream.observe( asyncStream )

// End observation later on.
observer.end()

Why

The standard approach of stream.map(() => m.redraw()) doesn't account for async streams.

Is this something you're interested in implementing yourself?

Yes.

Open Questions

  1. Should this behavior be split (e.g. stream.observe / stream.observeSync or inversed) in order to provider an idiomatic wrapper around the stream.map(() => m.redraw()) pattern without the async overhead?

  2. Should the redraw callback be exposed or is it better to assume it will always be m.redraw?

  3. Is there a performant Promise.all polyfill for IE? Otherwise if IE support for this particular method is dropped then this code can also be rewritten to use async/await instead of a callback.

@soulofmischief soulofmischief added the Type: Enhancement For any feature request or suggestion that isn't a bug fix label Dec 12, 2021
@StephanHoyer
Copy link
Member

This could easily live in userland. I tend to close this 🤔

@dead-claudia
Copy link
Member

dead-claudia commented Feb 21, 2022

I feel this should be split into two separate proposals, one for async .map and another for binding streams to m.redraw. Both ideas have merit, but it'd be a lot cleaner and more helpful to separate them and discuss them separately. (The rest of this comment will make that clear.)

This could easily live in userland. I tend to close this 🤔

@StephanHoyer Ordinarily I'd say yes, but this one could be reasonably defended:

  • Supporting an async variant of .map would make a number of streams a lot easier to write. (I would not support extending this to other operations, though - only .map.) While it technically could live in userland, it's a pretty common need with streams, probably more common than even filtering. Plus, the code isn't all that complicated:
    function asyncMap(stream, f) {
        let result = Stream()
        result.end(stream.end())
        stream.end.map(result.end)
        stream.map(v => {
            new Promise(resolve => { resolve(f(v)) }).then(result)
        })
        return result
    }
  • Binding streams to m.redraw could be done as a component-level thing to offer a unique mechanism to glue redraws together semi-implicitly. The code required would be relatively little on our end, essentially if (currentRedraw) stream.map(currentRedraw) on create and if (vnode.state.stream) hooks.push(callHook.bind(vnode.state.stream.end, vnode, false)) on remove.

I'm leaning in favor, but I don't plan to push for it personally as I don't really use streams much. Edit: After some discussion with other core collaborators, I've partially reversed - I'd be in favor of the first provided the streams module gets put into its own dedicated package, but I would also be not in favor of the second at least as of now.

@JAForbes
Copy link
Collaborator

I vote userland for numerous reasons. I think @nordfjord said it best:

The fact is we all have different use cases for converting promises, sometimes we need a loader, sometimes we want to maintain order, sometimes we just want all promises to push to a result stream.

paldepind/flyd#194 (comment) and discussion in paldepind/flyd#167

But a short list why I think this shouldn't be added:

  • It is super easy to pipe a resolved promise into a stream (.then(someStream))
    • Doing so avoids a lot of ambiguities that mithril shouldn't resolve on behalf of the user
  • The use case that requires this utility is making requests inside stream.map/etc
    • IMO this is an anti-pattern because side effects should occur at the edge of a reactive graph, not within it
    • But, if a user wants to do this, they can use .then(someStream)
  • Us including the ability to easily convert streams to promises via auto flattening tacitly encourages awkward usage patterns
  • Flyd arguably is the originator of mithril streams, and they walked back auto flattening in favor of very small focused utilities, we should probably follow suit.

@StephanHoyer
Copy link
Member

Ok, then we close this. Hope it's ok for you @soulofmischief

@soulofmischief
Copy link
Contributor Author

@StephanHoyer Sounds good to me.

So one example of where I'm using this is to asynchronously fetch the total number of results from a query, after updating another stream containing the query string.

const total = stream.lift(() => fetchTotal(), query)
stream.observe(total)

It seemed like a straightforward extension as my query was already a stream (following the bidirectional input binding pattern in the docs).

Would you recommend not (ab)using streams in this way? I can see how it would be an anti-pattern but I liked the terseness and clarity.

@JAForbes
Copy link
Collaborator

@soulofmischief it is not abusing it, it just pulls mithril in a direction that would be out of scope. If you are using a library like Rx/most.js/xstream etc it is pretty normal and encouraged to model everything as observables, and avoid subjects (in Rx parlance). But that necessitates having a lot of operators to model all the common/general ways to interact with n streams.

Flyd and mithril treat their streams as more of a channel, it is fine to write to a stream in mithril or flyd, and because it is encouraged, it means you don't need as many operators, as you can just implement them in situ by writing to the stream yourself.

E.g. Rx might have fromEvent but in flyd or mithril you'd go:

let click$ = stream();
window.addEventListener('click', click$)
click$.end.map( () => window.removeEventLIstener('click', click$) ) 

Rx is handling error, clean up, replays, parameterized schedulers, etc. And they have operators for everything imaginable to ensure you almost never need to write to a stream. In that world, it makes sense to flatten promises into streams and have operators that flatten in different ways. It also helps Rx observables have an error channel.

To start to go down this path would lead to complexity and would tacitly endorse that complexity, which isn't in the spirit of mithril (I think).

And that isn't to say complexity is bad, it just often isn't needed, or can be avoided.

But it is not an anti-pattern, or abuse in anyway. I recommend if you want to explore those patterns, try out xstream, callbags, Rx, most etc. All great libraries. That approach to reactivity will lead you toward needing more operators, but all those operators are covered by those libraries. Arguably those libraries are all more "pure", but there is a complexity cost.

Alternatively check out flyd and their promise conversion utils which is probably an ideal middle ground.

Mithril works great with all these options, but mithril's stream offering is deliberately lightweight.

Hope that helps :)

@soulofmischief
Copy link
Contributor Author

soulofmischief commented Mar 21, 2022

@JAForbes thanks for reaching out, I appreciate the recommendations, I will check out the other libraries you mentioned. I already use Rx a good bit which is another reason I embrace mithril streams. Observables definitely keep things organized.

What I did a couple months ago was just make a personal library for these stream extensions. I think I'll add a fromEvent inspired by the snippet you provided. If it ever gets large enough one day I'll pop into gitter and see if I can get some optimization advice / official endorsement.

By the way, a little off-topic but I checked out your website; love the music, good blog content as well. I think we have much in common and would like to chat a bit, what is the best way to reach you that isn't Twitter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Status: Closed
Development

No branches or pull requests

4 participants