Solvedmithril.js Criticisms of Mithril's stream architecture

First of all, I need to say, I mean absolutely no disrespect criticizing the architecture of the rewrite. I know how much experience everyone involved has, and on any other topic I'd defer completely to that experience. I'm excited that mithril 1.0 will have streams, and I think it will become a big part of the community. But I have legitimate concerns that come from my experience with using them in the context of mithril 0.2x.

I believe the current architecture has taken some missteps simply because the API has evolved from Promises + props, to just streams. Some of the patterns from the past don't make sense in this new context, and taking inspiration from other libraries has to be put in the proper context.

I'm concerned 1.0 is going to ship with a stream architecture that will be awkward to use. I think this stems from assumptions about stream usage that are incorrect.

I want us to really start from 1st principles. What problems do streams solve? What problems don't they solve? I want us to choose the best API for the common case while also retaining flexibility. I want us to avoid introducing confusing non standard concepts. I want us to be able to avoid pulling in dependencies to get substantial benefit from streams.

At a high level I propose:

  • m.request continues to return a promise.
  • streams shouldn't have absorption behaviour at all
  • streams shouldn't have error states.
  • map should be an instance method on the stream
  • merge should be included by default as m.prop.merge

I've come to these conclusion after using streams extensively in the context of mithril 0.2x.
I've witnessed common mistakes, and found patterns that work. I see the current architecture will
drive users of the library towards unintentional awkward patterns.

I believe it'd be better to remove streams entirely than ship with the existing API. I know that sounds hyperbolic, but I think a stream without a map method pretending to be a promise will leave a sour taste in people's mouths, at least without a built in stream we can always pull in a library.

There are problems that need to be solved in my proposal. For example if m.request continues to return a Promise, is it a native Promise? would that require dropping support for older browsers? I don't have answers to these problems, but I want to make my criticisms known so we can have that conversation and release a stream architecture that makes sense in the context of mithril.

Overview

  • m.request should return a Promise not a stream
  • run shouldn't exist
  • map should continue to live on the stream instance
  • we need a built in merging/sequencing operator
  • static operators are awkward without currying and a compose function

m.request should return a Promise not a stream

I understand wanting to get value for your LOC. Retaining promises in 1.0 while also introducing streams seems wasteful. But in practice, they are used very differently, and making m.request return a stream is just going to push people towards awkward patterns when they want to have repeating requests bound to that same stream.

const users = m.request(...)

// later on I want to fetch users again
// so I use the original stream as a callback
// so there are 2 places that define users data flow now instead of 1
m.request( ... ).map(users)

If m.request continued to return Promises, that code would look something like this:

const users = m.prop()
const fetchUsers = () => m.request(....).then(users)

// later on I want to fetch users:
// I'm only defining behaviours and data flow once
fetchUsers()

You could argue the exact same pattern is possible with streams if you treat m.request as a defacto Promise:

fetchUsers = () => m.request(...).map(users)

But at that point you really have to wonder why m.request returns a defacto Promise, instead of a Promise. The fact it is a stream, is just going to confuse users. And the fact the stream is pretending to be a Promise, means the stream has to have more complex semantics
(error states, absorption). These are concrete solutions to theoretical problems that don't really exist.

run should be dropped

run exists because there is a theory that in real world apps we'll write nested streams when requesting data. Again, this stems from treating streams like promises. Here is an example of the theoretical usage.

m.request({url: "/api/users", method: "GET"})
    .run(function(users) {
        if (users.length === 0) return m.prop.reject("No users found")
    })
    .catch(function(e) {
        console.log(e)
    })

The reason we need run here, is because streams in 1.0 have error states, and the way we trigger that state is to return an error stream. Now we have a nested stream, so we need run to automatically flatten the nesting.

m.stream.reject, isn't a stream, its a value. It's not even asynchronous. Why are we creating a nested stream architecture to support a value that isn't even a stream?

This all stems from treating streams like Promises. If m.request returned a Promise (as it used to) streams would not need error states, and we wouldn't have a need for run anymore.

Treating streams as Promises is making the stream design more complex than it needs to be.

We don't need run, we don't need error states.

Flyd's solution is to auto absorb Promises, but we don't need to do that (even flyd is looking to drop support for Promise absorption). I use streams a lot and I've never had a case where there was a Promise in a stream. Its all theoretical. The entire design is theoretical.

const users = m.prop()

// we will only ever write to the stream if the request didn't fail
const fetchUsers = () => m.request(...).then(users).catch( ... )

// we can create a Boolean stream that indicates if the list is empty
// this is contrived, but the idea is, we can create streams for special states
// that will affect conditional rendering.  We don't need to build states
// into the stream model itself.
const userError = users.map( users => users.length == 0 )

// we can use both data sets to conditional render
const view = m.stream.merge([users, userError])
  .map(
    ([users, userError]) =>
        userError 
        ?  m('.error', 'You do not have any users yet.  Please create one')
        :  m('.success', 'You have '+users.length+' users')
  )

I'll go into why I think we need merge later in this issue.

This is a more extensible architecture. Treat Promises as Promises, treat streams as streams. Streams suddenly become a lot simpler (no error states, no run, no nesting/absorption), and you're encouraging patterns that are more declarative.

  • Streams will be much simpler to understand, because their semantics are simpler
  • Promises are a part of the language now, so avoiding them is more confusing than including them
  • Refactoring to use other libraries (e.g fetch API) will be easier, because we aren't reinventing promises

The existing architecture only makes sense if you never want to fetch the same data twice. I know from experience that treating streams as Promises becomes awkward quickly.

map should continue to live on the stream instance

@lhorie cited this discussion when I questioned the removal of map from the stream instance.

I assume you are taking most of your inspiration from Simon's comments.

Quoting paldepind:

I've created a library that exposes reactive streams that implements the applicative interface. This means that the streams must have a map method. I've only added the map method to support fantasy land. My users should not use it directly, they should instead use the map function I make available. However since object oriented programming is so common in JavaScript people continue to get it wrong even though I've added a disclaimer in the documentation. This confusion would probably never happen if the method names where prefixed.

It is very annoying that one has to implement a object oriented API that can easily be mistaken for a genuine OO API in an otherwise completely functional library that makes no use of methods.

Source

I agree with Simon's perspective, the trouble is composing functional API's becomes extremely messy without a function available like R.compose/R.pipe/_.flow/_.compose. Including compose in mithril isn't enough, you also need all your static api's to take the data last and to be curried. Otherwise your composition pipelines require constant arrow wrapper functions to pass the stream into the operator. I believe one of Mithril's core principles is the ability to build a sophisticated app without requiring any dependencies, while also having a minimal API surface. If you want to take Simon's approach, you need compose, and you need currying.

I'll post the code sample I wrote when making this argument in the gitter chat:

const f = compose(
  map( multiply(100) )
  map( prop('someProp') )
  dropRepeatsWith( eqProps( 'someProp'))
)

const a = m.prop({ someProp: 100 })
const b = f(a)

The above code takes a stream of objects with a property someProp, and returns a stream that emits only when the object's property has changed, extracts that property, and multiplies it by 100.

Its concise, elegant, and its exactly the way Simon want's his users to use flyd. But notice that map and dropRepeatsWith are all curried. They are all waiting for the final argument, the stream.

I don't think mithril should include a compose function, I think it will just confuse new users, but let's say for arguments sake, mithril included a compose fn, we'd still need to account for the fact the operators aren't curried.

const f = m.compose(
  s => map( multiply(100) , s )
  s => map( prop('someProp') , s)
  s => dropRepeatsWith( eqProps( 'someProp'), s)
)

const a = m.prop({ someProp: 100 })
const b = f(a)

The code isnt unreadable now, but its noisy, its not elegant. Its also error prone, all it takes is for someone to forget to pass s to a stream and their composition pipeline is broken.

Now let's imagine this code sample without a compose operator.

const a = m.prop({ someProp: 100 })
const b = 
    map( multiply(100) 
      ,map( prop('someProp') 
         ,dropRepeatsWith( 
             eqProps( 'someProp'), a
         )
      )
    )

It's almost unreadable, adding/removing new operators requires nesting/denesting the entire sample. This API discourages composition, which in my opinion is the worst thing an interface can do.

Let's add map back to the instance, and see what we get.

const b = 
  a
   .map( prop('someProp') )
   .map( multiply(100) )

const c = 
  dropRepeatsWith( eqProps('someProp'), b) 

It's not perfect, but it handles the common case (transformation) elegantly. The common case with streams is transformation. By keeping map on the instance, we can do the majority of the work in one place without creating complex nested structures. I think this is a fine compromise. Give users of mithril the tools to handle 90% of cases without pulling in any libraries or modules.

I think everyone in this community knows how much I like Ramda, currying and composition. But I also like mithril's philosophy: you can see it repeated again and again in this thread

A tiny, elegant API surface frees you to think about solutions to your problems instead of trying to frame them in the language of the framework.

@barneycarroll

no fucking magic

@StephanHoyer

Batman's utility belt - Everything you need at arms length, but you barely even notice it.

@tropperstyle

Everything is vanilla functional JavaScript. Refactoring becomes easy and enjoyable.

@reminyborg

I've just taken the first few comments from the thread. If you read the comments in the context of this stream architecture I think a clear pattern emerges. Users of mithril don't want to pull in R.compose to compose static operators. Users of mithril don't want magical complex behaviour like currying, or stream absorption, pseudo promises.

Keep .map on the instance, remove all the psuedo promise behaviour and you have a fine extensible API.
If I'm wrong, its easy to add run and error states later. I am probably one of the most experienced using streams extensively in the context of mithril. We won't need these features.

We need a built in merge/sequencing operator

I would like some basic operators on every stream instance. An operator that allows transformation (map), an operator that controls whether or not to emit values (filter), and an operator that allows creating streams that depend on multiple sources (merge).
If mithril exposed reduce, we could define hundreds of other operators in terms of it.

But additional functions need to be weighed against mithril's small API surface. So as an absolute minimum, I think every stream needs a map method, and we need a merge operator. Everything else can be a static function and our code will still be concise and easily refactored. Most stream code is transformation and merging.

Streams are about creating relationships. Streams are a lot like functions, they can receive many values, but always returns a single result. The majority of a stream's usage is simply transformation, but its essential to create streams that depend on other streams declaratively. Because mithril implements fantasyland/ap, we can take advantage of functions in Ramda (lift, sequence, traverse) to achieve merging. But I think this functionality is so basic, we should have it included by default without any dependencies.

I don't mind what the signature is, it can be variadic, it can be an array of streams. I don't think it needs to live on the instance. But it should be included by default. Without it people will reach for combine, which is there for efficiently creating operators, not for application code. Writing functions that accept streams instead of plain values is not only noisy, but it locks the dev into a particular stream API for no good reason.

const a = m.prop()
const b = m.prop()
const c = m.prop()

const sum = 
  m.prop.merge(a,b,c)
    .map(
      (a,b,c) => a + b + c
    )
  
// vs

m.prop.combine(function(a,b,c) {
  return a() + b() + c()
}, a,b,c)

The function body has nothing to do with streams, so relying on combine is mixing concerns. It's also likely people will forget to call the streams.

m.prop.combine(function(a,b,c) {
  // a likely common bug when using combine
  // in app level code
  // we're forgetting to call a,b,c
  return a + b + c
}, a,b,c)

Conclusion

I think we should have simpler stream semantics. I want us to separate Promise behaviour from stream behaviour and remove run in the process. I think we need to be able to merge streams easily. And I think depending on static operators for basic functionality (map/merge) doesn't make sense in a dependency free VanillaJS context.

My criticisms will raise questions that I don't pretend to have answers for. I know the existing API is the way it is for a reason. I know it solves other problems that my proposal's don't. But I think this architecture isn't ready for prime time yet. It needs to be iterated on further.

Thank you for your taking the time to read this, I'm sorry it is such a long post, architectural argument's are subtle and hard to compress.

33 Answers

✔️Accepted Answer

Implemented changes as of 6ce2a38:

  • m.request / m.jsonp return Promise
  • Streams are now simplified (no .error stream, no .reject, no .catch, no .run, no absorption semantics
  • stream.map is an alias of stream["fantasy-land/map"]
  • Streams are no longer part of the core distribution
  • docs are updated

Other Answers:

@barneycarroll Re: m.request(url) - I'm neutral to it. If someone creates a PR for it, I'll merge it.

Generally: it sounds like most people here are in the "m.request should return a promise" camp (and frankly, with good arguments too). My rationale for why m.request returns a stream is largely that I feel that streams are a more powerful "superset" of promises, but, as I mentioned, the arguments against that are also strong. In light of that, I think it would make sense to revisit that decision and have m.request go back to returning a promise.

In that case, here's what I would envision would happen:

  • the Promise polyfill would be part of core (until IE end-of-life)
  • the return value of m.request would be a Promise (and a Promise would not also be a prop)
  • streams would not inherit semantics for absorption (i.e. .run() et al) nor error states since those primarily exist to mimic promise semantics
  • streams would have an instance method map

Here are possible changes that I think are debatable:

  • a) removing stream from the core distribution (possibly in favor of some simplified-redux-like architecture) and instead exposing streams as a separate module (or more likely, a collection of modules, if there are static operators)
  • b) possibly adding a dispatcher helper, to support a)
  • c) streams would have a merge (basically a nicer combine), or filter and reduce (whichever is deemed better)

From my (somewhat limited) experience w/ redux, a dispatcher-based architecture appeared to yield less code (both in framework space and app space), and felt simpler to understand (to me, at least) than both the m.prop marshalling/unmarshalling oriented approach and the stream-oriented approach. Note that when I say "dispatcher-based architecture", I don't mean Redux itself (in fact I felt it was excessively boilerplatey for my use cases, even in its least verbose variations)

@JAForbes Just to clarify, when you talk about merge, what exactly is its definition? (I'm asking because there is already a merge static method, so I'm trying avoid confusion)

This was a beautiful sequence of messages to witness. Loving the community around Mithril.

I agree with @nordfjord: I think it's slightly obnoxious for Mithril not to use the promise contract as its core async API. The 0.X m.request implementation is simply better than the 1.X one IMO.

Promises are great because they are the core standard API for async interactions in Javascript. It's true that older engines don't provide a native Promise object, and await / async has yet to land in most engines. But none of that casts the stream API in a better light.

The only decent argument against implementing a thenable m.request in 1.X is that streams offer reusable potential that conflicts with the Promise specification that any specified then & catch will execute at most once.

Can't we have our cake and eat it, by overloading m.requests to offer then as well as map?

As @JAForbes says, the long 'why return streams' section doesn't really provide an argument against Promises: I read it as saying that it often emerges that separating persistent properties and the actions that populate them is a really good idea. I found this to be the case in Mithril 0.X, where I discovered that providing initial values to m.request calls read very badly, and fell apart once I discovered I needed to make new requests (or reset the initial value).

var articleListController = function(){
  this.articles = m.prop( [] )

  this.reset = () => this.articles( [] )

  this.fetch = query => m.request( { method : 'GET', url : 'api/articles/' + ( query ? '&query=' + query : '' ) } ).then( this.articles )
}

As you can see, the eventual point of the 'why return streams' is orthogonal to streams. In fact, it's a great argument for keeping separate action and data persistence, which could be read as strengthening the idea of Promises for requests and streams for props.


@lhorie is right in asserting that fetch API has plenty of inadequacies, and using XHR (or JSONP) behind the scenes keeps m.request more versatile. I still believe the potential terseness of generic fetch signatures is an awesome default, and could be incorporated into m.request - such that m.request( url ) provide a shorthand for m.request( { method : 'GET', url } ).

@darsain bundling Promise is a trade-off. You're right that you'd end up with 10 different implementations if you had 10 libraries with their own implementations, but to be fair, that's not necessarily likely to happen, and the alternative is a requiring a configuration step just to get to a point where you can make your first xhr request (and that might not even be the same step depending on whether you're using webpack, babel or jQuery). Given that Mithril is supposed to be a utility belt of sorts, I don't think it's unreasonable for it to ship Promises in the interest of decreasing configuration fatigue.

Also, as @pygy mentioned, you would be able inject your own Promise if you require modules piecemeal rather than wholesale require("mithril")

@nordfjord the polyfill in rewrite is fully A+ compliant (and for that matter, has ES6 Promise API parity as well)

More Issues: