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

Criticisms of Mithril's stream architecture #1391

Closed
JAForbes opened this issue Nov 7, 2016 · 32 comments
Closed

Criticisms of Mithril's stream architecture #1391

JAForbes opened this issue Nov 7, 2016 · 32 comments

Comments

@JAForbes
Copy link
Collaborator

JAForbes commented Nov 7, 2016

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.

@mpfau
Copy link
Contributor

mpfau commented Nov 9, 2016

+1

From my experience, mithril really excels in ease of use. The only exception for me are streams. I like the idea to simplify the stream semantics as they are hard to understand. That is why I stopped using m.request in favour of a custom XHR-Wrapper (based on promises).

I also think that mithril should provide a very simple Promise polyfill that can be replaced in a simple way (e.g by loading bluebird beforehand), as native promises are still missing from IE 11.

@tivac
Copy link
Contributor

tivac commented Nov 10, 2016

👍 on m.request returning a promise again.

I'm deeply conflicted on having streams in mithril at all.

@dead-claudia
Copy link
Member

Regarding streams, I'd agree to a moderate degree. I'm not sure how
necessary the abstraction is in core, at least at the basic level. Most of
the core API doesn't even use them, so I'm not convinced they're actually
necessary in core. (I'll admit their existence was surprising when I first
saw them with the rewrite early on.) But I'll taper off so I don't drive
this too off topic.

@lhorie Thoughts on this? (either the m.request proposal here or my note
on streams)

On Wed, Nov 9, 2016, 20:16 Pat Cavit [email protected] wrote:

👍 on m.request returning a promise again.

I'm deeply conflicted on having streams in mithril at all.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1391 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AERrBE10keMQXR_tbtixXj4DYAWYeMhMks5q8nB9gaJpZM4Kqzuu
.

@nordfjord
Copy link
Contributor

nordfjord commented Nov 10, 2016

I agree that m.request should return a promise. I also think mithril shouldn't need to have it's own Promise implementation at all. If you want to support legacy browsers supply a polyfill yourself.

And let's be honest. If you're supporting legacy browsers you're most likely already including a promise polyfill.

@darsain
Copy link
Contributor

darsain commented Nov 10, 2016

request() definitely should return a promise, but than you've just re-implemented fetch(). So, to keep up with the minimal API surface area recommendations (which I found again and again to be very valuable in practice), just drop request() in favor of fetch() polyfill and be done with it.

@tivac
Copy link
Contributor

tivac commented Nov 10, 2016

The intent behind mithril is to ship a small library that can contain the majority of the pieces necessary for building a SPA, so I don't think stripping out m.request is a good idea. It introduces additional burden on people who just want to get moving w/o having to go figure out the best fetch polyfill. Same for Promise support, really.

If you don't need those then awesome, you can include just the pieces of mithril you want into your bundle and you're good to go.

@spacejack
Copy link
Contributor

Personally I find request with its auto-redraw is a definite nice to have, along with its JSON by default behaviour. Overall I really like the request API.

Returning a promise would likely feel more 'natural' to a newcomer to the framework. I'd be okay with having to supply my own Promise polyfill if needed; I'm not sure that Mithril should need to do that for you.

I also like having streams (or some form of simple databinding object via m.withAttr.) I'm not sure how much I'll get into the more sophisticated uses for streams, but it'd be nice to have that to explore in future.

Just some layperson's thoughts. :)

@nordfjord
Copy link
Contributor

nordfjord commented Nov 10, 2016

@tivac You could just as well argue that you're placing additional burden on people by making them learn a new API which differs from the standard. Take for instance the fact that [email protected] promises are not A+ compliant which has caused confusion for many people (including me). fetch is the new standard for making requests and most people seem to find it alright.

@spacejack
autoredraw from fetch requests can easily be achieved in userland e.g. something along the lines of

function request(){
  return fetch.apply(null, arguments)
  .then(res => res.json())
  .then(data => (m.redraw(), data));
}

@sdougbrown
Copy link

Honestly I kind of like that m.request shares the stream API - it allows you to substitute one for the other without extra indirection (i.e. for a simple caching layer).

I have found combine to be clunky in its current form and support the idea of merge.

@lhorie
Copy link
Member

lhorie commented Nov 10, 2016

Sorry for the late reply

@JAForbes you brought up some good arguments. I'm not against reinstating map as a method if you feel that makes more sense. If we were to go the route of making streams more "batteries-included" (in the sense of having more useful instance methods), which methods would you like to see?

Re: combine - I literally stole that from flyd and I'm not particularly married to it. It's a really low level construct, and I'm not at all opposed to exposing a higher level alternative.

Re: m.request vs fetch: I had some thoughts written on the subject here: https://github.com/lhorie/mithril.js/blob/rewrite/docs/request.md#why-xmlhttprequest-instead-of-fetch (and for that matter, the following section are my thoughts on promises vs streams)

@JAForbes
Copy link
Collaborator Author

@lhorie thanks for taking the time to read it all :)

In short

  • I think we should add map back
  • Lets have a built in static merge
  • I still think m.request returning a Promise retains the benefits you mention in your post
  • I think we should drop run (but that's contingent on m.request returning a Promise)
  • We should keep combine but only use it for creating static operators efficiently, not for app code
  • Let's not add a lot of instance methods, just enough for the common case

I'm going to elaborate a bit below, but its much the same as what I said above.

Promises vs Streams

I agree with out points regarding promises vs streams. I think though you get much the same benefits by using m.request as purely data acquisition and then funnelling that immediately into a separate stream pipeline. In your own example this is exactly the tactic your taking:

findProject(123).run(project)

We've got one process (data acquisition) where we fetch the data. And we've got another process, reactive transformation/filtering which is what streams are so good at.

We need to separate the two systems so we can have repeat requests that feed into the same pipeline. So at that point m.request returning a stream doesn't actually carry any benefits.

I'm going to go on a tangent to point out when m.request returning a stream would make more sense to me. Feel free to skip if you already see the point I'm making in the summary...

m.request returning a stream would make more sense on a pull model

It is no accident that separation is the best tactic. We want to fetch new data from the endpoint many times, so we need to define the pipeline separately so we can push new data in later.

Mithril streams work on a push model. Mithril streams are what most/Rx people would call subjects. We push data into project which triggers a reaction instead of pulling data from the server through a pipeline of transformation. This subtle difference has ramifications for the return type for m.request.

If we had a pull model then m.request returning a stream starts to make a lot more sense. We could define project as:

const project = m.request(...)
  .map( a )
  .map( b )
  .map( c ) 
  .pull()

And when we need a new data-set, we could pull it again.

onclick: () => project.pull()

Which would essentially re-execute m.request, and all the transformation functions (a,b,c) whenever we pull.

I'm not saying we should use pull streams, (in fact I don't think that is a good idea at all for other reasons) but I'm saying that is the 1 time where m.request returning a stream would make sense to me.

I still think m.request should return a Promise, and I think the advantages you mention in the docs would still stand. Relieving streams of the responsibility of Promises will make streams easier to grasp, I imagine too it will involve less code (no error states, no run/absorption code).

XHR vs Promise

I agree m.request does a lot more than a fetch polyfill in less code - I think building on top of XHR has advantages I know there are many concerns to balance but I see the m.request discussion as separate to the stream discussion.

I don't have much skin in the m.request game because I do use fetch, but I see the value in m.request for the community.

But m.request returning streams has an affect on the stream API which is why I'd prefer m.request to stick with Promises. I use streams for all sorts of things, network traffic is only a slice, and I feel that m.request needs Promises, but all other the cases where I use streams do not need error states or absorption. I guess I want to localise that requirement to m.request because streams are a more general purpose tool.

I use streams for handling events, dom manipulation, caching (indexDB, localStorage), cross component communication, url changes etc. The list keeps growing, but I don't need error states for any of those cases and I'm assuming other people would find themselves in a similar situation as well, post 1.0 release.

And its not that having run or error states actively hurts me, it is just an unnecessary part of the API and I think we all want the same thing; for mithril to have the minimal required API surface without it being painful to use. I think run doesn't make the grade.

Batteries included

We should tread carefully when adding instance operators, I agree with Simon (paldepind) that static operators are better than instance operators as soon as you do bring in other libraries that can handle composition.

I really want the bare-bones mithril app to be able to cover the common stream usage case with 0 libs. And I think that common case is map and merge. I am definitely opposed to a batteries included stream instance like Rx (with 100's of methods). The upper limit on (non FL prefixed) instance methods I'd support would be map, filter and reduce. The lower limit would be map, and a built in static merge in either case.

Once people reach the limits of that common case you'd probably be reaching for 3rd party streams or fantasy land libraries like Ramda to do some more advanced stream work. So adding more batteries just serves to bloat mithril. But map and merge really are the most important features in my view. And I think map needs to be the only method on the instance.

@barneycarroll
Copy link
Member

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 } ).

@lhorie
Copy link
Member

lhorie commented Nov 11, 2016

@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)

@darsain
Copy link
Contributor

darsain commented Nov 11, 2016

the Promise polyfill would be part of core (until IE end-of-life)

No, please don't. No module should be registering it's own proprietary polyfills. What if everyone that needs promises did that? Now my build file has 10 polyfills fighting for one global property.

Just rely only on the current features in the spec, and let us include our own preferred polyfill solution.

@lhorie
Copy link
Member

lhorie commented Nov 11, 2016

@darsain You could still do that, for example by using resolve.alias in webpack

@pygy
Copy link
Member

pygy commented Nov 11, 2016

Alternatively, you can provide a mithril/promise module that returns the global Promise if present and a compliant polyfill otherwise.

@darsain
Copy link
Contributor

darsain commented Nov 11, 2016

@lhorie so people have to hack around your unconventional package distribution decisions? 😢Do I have to scan each module I'm using to check if it's not bundling polyfills so I can alias them out? Or worse, fork it when they're inlined in?

I don't want to be complaining constantly, but these are strange decisions. It feels like you are optimizing for using mithril as a standalone build file via script tags, but we stopped doing front-end like that 5 years ago.

Besides, I don't use webpack 😳.

@pygy that wouldn't work. As long as any part of core requires mithril/promise the builder has to bundle it in. The only way how that could be made optional is if the user calls mithril/promise before anything else, which than registers global Promise, but by that point you should just recommend some promise polyfill module and not reinvent the wheel all the time.

@pygy
Copy link
Member

pygy commented Nov 11, 2016

@darsain I suggest that the polyfill only ever reads the global Promise, never writes to it. request/request can then depend on promise/promise.js. It has to be bundled, but environments that provide a global Promise (either native or user-defined) will get a m.request that depens on it, environments that don't will get one based on the (A+/ES6 compliant) polyfill.

@porsager
Copy link
Contributor

Is there any problem in doing?

m.Promise = Promise || PromisePolyfill

That would also make it easy for anyone to fill in something like bluebird by simply doing

m.Promise = Bluebird

Yes it would put on a few extra kb until @lhorie thinks it can be pulled out, but if those kb's matter you can build/require mithril without it?

@nordfjord
Copy link
Contributor

nordfjord commented Nov 11, 2016

I'm fine with a Promise polyfill so long as it's standards compliant (that means a setImmediate polyfill is required). If it's not standards compliant it will result in users having to wrap the promises in stead of them being gracefully absorbed.

That being said, the issues we have with Promises and inclusion of polyfills in core really belong in a seperate issue from this one which has to do with the stream API.

@darsain

but we stopped doing front-end like that 5 years ago
Besides, I don't use webpack

You can achieve the same result using browserify-shim if you're using browserify. Most module loaders allow you to define aliases in one way or another.

@jonahx
Copy link

jonahx commented Nov 11, 2016

Apologize for the clueless question, but where can I find discussion of the new stream architecture and a test build? I didn't see anything on the site. Thanks.

@lhorie
Copy link
Member

lhorie commented Nov 11, 2016

@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)

@barneycarroll
Copy link
Member

I've written a fetch-like overload for m.request in #1393 with tests but no documentation. I think the mechanics make sense and I'd like some eyes on it to help validate what kind of write up we want for that.


The Promise polyfill is a tough issue. In many ways this highlights the broken dream of zero-sum hyper-modularity and race to the bottom byte-size economy.

@nordfjord setImmediate polyfills are simply not possible without a native Promise.resolve - deferred tasks cannot be given any higher precedence without either of these APIs. Mithril 0.X used to eagerly resolve m.requests synchronously, which is a terrible violation of contract. I argued with @lhorie that the idea of must-occur-within-4ms is not part of any desirable spec for a library like Mithril. XHR will resolve on its own queue. If that happens within 4ms or later, that's it's own issue and you have to be prepared for that. The idea that promises should resolve instantly is wrong; the idea they should internally resolve immediately and then happen before the rest of the events queue is dubious at best.


I tried a (native) Promise aliasing fork to patch then onto requests, but got some difficult to debug erros WRT 'possible uncaught errors'. I'll keep at it. For now I'll stick to native Promises, but I'll include the polyfill in the codebase if that's desired.

@pygy
Copy link
Member

pygy commented Nov 11, 2016

Native promises have microtask semantics (they can starve the event loop), while setimmediate doesn't (it schedules one task per tick at most).

Beside promises, you can harness microtasks using MutationObservers, but they are not available before IE11, and even then there's this mess, which means we can't get consistent execution order across browsers.

I don't think you can get microtasks in IE 9 and 10. For tight macrotasks, you can use a subset of the setImmediate polyfill that targets these two browsers, though. I actually would like to experiment with that approach in the router to get the same timing with both the onhashchange (IE9) and onpopstate implementations.

@JAForbes
Copy link
Collaborator Author

JAForbes commented Nov 12, 2016

@lhorie Sorry I didn't know there was already a static merge method. My apologies.
I'll explain what I am imagining merge should be anyway.

merge would just wait for all streams to have a value before emitting. From then on if any stream receives a value it emits again.

As for the actual signature, that is debatable, I think that's worth discussing.

I'd vote that mithril should sidestep type safety and err on the side of convenience

I think a variadic signature with no currying and an included callback that receives a spread of values:

(fn, ...streams) 

Where fn receives a spread of the values of all the resolved streams.

In practice:

var a$ = m.prop()
var b$ = m.prop()
var sum = m.prop.merge( (a,b) => a + b, a$, b$ )

sum() // undefined

a$(2) 
sum() // undefined
b$(3)
sum() //5
b$(4)
sum() //6

The above is basically R.lift( fn, ...streams) with some minor caveats (currying, immediate execution)

I personally would prefer a different API, but I think the above signature would probably be best for the 0 dep, es5 scenario. It encourages using simple functions that are unaware of stream semantics, and it doesn't require the dev to spread an emitted list of values like R.sequence would.

Worth mentioning people can take a more precise route by using R.sequence, R.traverse, or R.lift because we still have fantasyland/ap on the instance. And by precise I mean, accept an array of streams and emit an array of values with no callback.

If they go the Ramda route they could switch to another stream library mid project and their existing code would transfer over. I'll definitely write about how to take advantage of that approach post 1.0 release for anyone who is interested. But I think its really important that is just a bonus, not a requirement.

Here is the "precise" approach:

// List Stream a -> Stream List a
// Or in Typescript:
// Stream<T>[] => Stream<T[]>
var merge = R.sequence(m.prop)

var a$ = m.prop(3)
var b$ = m.prop(5)
var merged = merge([a$, b$])

merged() //=> [3,5]

// this is how we'd spread the values in the context
// of ramda + sequence
var sum = map( apply(add), merged )

sum() //=> 8

I personally prefer that API but its awkward if you don't have access to other utilities like apply or spread syntax.

I think the thing that really matters though is the behaviour not the signature. Merge should be thought of like Promise.all except that it continues to emit after all values have arrived.

@barneycarroll
Copy link
Member

@pygy are you aware of asap it looks pretty comprehensive, well thought out and small. The setImmediate and postMessage shortcomings are addressed, and it's awareness of the various engine implementations are well documented…

@pygy
Copy link
Member

pygy commented Nov 12, 2016

@barneycarroll I didn't know about it, and if you read further down, you'll see it can't do any magic on old IE either... For the oldest browsers it falls back to having setTimeout and and setInterval race then cancel the one that didn't fire first. Now that's commitment in the face of adversity!

@lhorie
Copy link
Member

lhorie commented Nov 16, 2016

@JAForbes the current merge works similar to Promise.all

var a$ = stream(11)
var b$ = stream(22)
stream.merge([a$, b$]).map(function(list) {
  console.log(list[0]) // 11
  console.log(list[1]) // 22
})

You're welcome to bikeshed on signature if you feel it could be improved

@JAForbes
Copy link
Collaborator Author

That's my preferred signature, so if you're happy with that I'm happy.

-----Original Message-----
From: "Leo Horie" [email protected]
Sent: ‎11/‎16/‎2016 1:36 PM
To: "lhorie/mithril.js" [email protected]
Cc: "James Forbes" [email protected]; "Mention" [email protected]
Subject: Re: [lhorie/mithril.js] Criticisms of Mithril's stream architecture(#1391)

@JAForbes the current merge works similar to Promise.all
var a$ = stream(11)
var b$ = stream(22)
stream.merge([a$, b$]).map(function(list) {
console.log(list[0]) // 11
console.log(list[1]) // 22
})You're welcome to bikeshed on signature if you feel it could be improved

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@lhorie
Copy link
Member

lhorie commented Nov 16, 2016

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

@barneycarroll
Copy link
Member

@lhorie thanks for this very judicious work, I think Mithril is far easier to adopt & more future-proof as a result. A lot of long-running concerns have been laid to rest here!

@lhorie lhorie closed this as completed Nov 16, 2016
@hugows
Copy link

hugows commented Jun 4, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests