-
-
Notifications
You must be signed in to change notification settings - Fork 927
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
Comments
+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. |
👍 on I'm deeply conflicted on having streams in mithril at all. |
Regarding streams, I'd agree to a moderate degree. I'm not sure how @lhorie Thoughts on this? (either the On Wed, Nov 9, 2016, 20:16 Pat Cavit [email protected] wrote:
|
I agree that And let's be honest. If you're supporting legacy browsers you're most likely already including a promise polyfill. |
|
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 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. |
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. :) |
@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). @spacejack function request(){
return fetch.apply(null, arguments)
.then(res => res.json())
.then(data => (m.redraw(), data));
} |
Honestly I kind of like that I have found |
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: 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) |
@lhorie thanks for taking the time to read it all :) In short
I'm going to elaborate a bit below, but its much the same as what I said above. Promises vs StreamsI 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 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...
|
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 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 The only decent argument against implementing a thenable Can't we have our cake and eat it, by overloading 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
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 |
@barneycarroll Re: 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:
Here are possible changes that I think are debatable:
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 |
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. |
@darsain You could still do that, for example by using resolve.alias in webpack |
Alternatively, you can provide a |
@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 |
@darsain I suggest that the polyfill only ever reads the global |
Is there any problem in doing?
That would also make it easy for anyone to fill in something like bluebird by simply doing
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? |
I'm fine with a 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.
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. |
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. |
@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 @nordfjord the polyfill in rewrite is fully A+ compliant (and for that matter, has ES6 Promise API parity as well) |
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 I tried a (native) Promise aliasing fork to patch |
Native promises have microtask semantics (they can starve the event loop), while Beside promises, you can harness microtasks using I don't think you can get microtasks in IE 9 and 10. For tight macrotasks, you can use a subset of the |
@lhorie Sorry I didn't know there was already a static merge method. My apologies.
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 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
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 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 I think the thing that really matters though is the behaviour not the signature. Merge should be thought of like |
@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 |
@JAForbes the current 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 |
That's my preferred signature, so if you're happy with that I'm happy. -----Original Message----- @JAForbes the current merge works similar to Promise.all |
Implemented changes as of 6ce2a38:
|
@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! |
This was a beautiful sequence of messages to witness. Loving the community around Mithril. |
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:
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
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.
If
m.request
continued to return Promises, that code would look something like this:You could argue the exact same pattern is possible with streams if you treat m.request as a defacto Promise:
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.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 needrun
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 neederror
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.
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.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:
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
. Includingcompose
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 needcompose
, and you need currying.I'll post the code sample I wrote when making this argument in the gitter chat:
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
anddropRepeatsWith
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.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.
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.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
@barneycarroll
@StephanHoyer
@tropperstyle
@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
anderror
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.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.
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.
The text was updated successfully, but these errors were encountered: