adds filter method to streams

[nordfjord]

I would love to see more "Array"-like methods on mithril streams, so I thought I'd start by implementing a filter method

[dead-claudia]

@lhorie @tivac I'm kind of feeling 👎 on this...sounds like a wrapper/library that belongs in user land IMO.

[nordfjord]

@isiahmeadows In my view streams are arrays in time. So supporting things like reduce, map, filter, and others makes sense. Whether or not it makes sense for mithril is another question entirely, and is up to you.

[gilbert]

How about a proof-of-concept wrapper snippet? (not that I would know how to write one)

[nordfjord]

@mindeavor That would be easier if the stream prototype was somehow exported

Then we could do something like:

m.prop.prototype.filter = function(f) {
  var stream = m.prop();
  this.map(value => f(value) ? stream(value) : null);
  return stream;
};

[adriengibrat]

Stream.stream = function(originalCreateStream) {
    return createStream

    function createStream() {
        var stream = originalCreateStream()
        stream.filter = filter
        return stream
    }

    function filter(fn) {
        var stream = createStream()
        this.map(value => fn(value) ? stream(value) : null)
        return stream
    }
}(Stream.stream)

wrapping ìs a bit more convoluted but works ;)

[nordfjord]

@adriengibrat I'd rather the streams be made so they can be easily extended rather than having to Monkey Patch them.

[adriengibrat]

I know and I understand your position. this was more a response to @mindeavor
Using prototype implies a complete rewrite of stream.js it may deserve another issue

[lhorie]

That would be easier if the stream prototype was somehow exported

Unfortunately, this is only possible in true ES6 environments (i.e. not via Babel) because the only way to make it work properly requires Function to be subclassable :(

@nordfjord Could you make a case for why a stream should have built-in array methods (and which ones)?

[nordfjord]

@lhorie I'm not sure I can make a better case than: because they are arrays. Normal arrays are arrays over space while streams are arrays over time. For me that means a similar, or the same, set of methods should be available on both. Whether that brings any greater usability to the streams is another matter, it's just a matter of principle for me since I tend to think of streams as being arrays.

I'd submit that a good starting point would be to have: map, filter, reduce, and find.

reduce is useful in a case like this, where we incrimentally update some state once we receive more data.

// assume a websocket connection that
// periodically sends you statistics
let pageviews = subscribeToPageviews()
.reduce((p,v)=> p + v.views, 0);

// ...

view(){
  m('.pageviews', pageviews())
}

filter can improve the case if say we only want female pageviews

let pageviews = subscribeToPageviews()
.filter(view => view.gender === 'female')
.reduce((p,v)=> p + v.views, 0);

find is useful in a case where you only want the first of a given filter.

let fraudObservation = subscribeToObservations()
.find(obs => obs.type === 'fraud');

// ...


view() {
  if (fraudObservation()) {
    return m('a.toast', {
      href: '/fraud-list?selected=' + fraudObservation().id,
      oninit: m.route.link
    }, 'Just got a fraud notification, investigate now')
  }
}

[nordfjord]

Unfortunately, this is only possible in true ES6 environments (i.e. not via Babel) because the only way to make it work properly requires Function to be subclassable :(

Would something like this not work, since you're not using real prototypes ATM?

function initStream(stream){
  Object.keys(streamMethods).forEach(k => stream[k] = streamMethods[k])

  // ...

  return stream;
}

var streamMethods = {map: map, ...}

module.exports = {stream: createStream, methods: streamMethods, ...}

var Stream = require('mithril/util/stream');

Stream.methods.filter = function(){
  //...
}

[lhorie]

@nordfjord thanks, that helps. Personally, I don't agree that streams are arrays in time (e.g. reduceRight doesn't work), but I can see the value of those methods

re: streamMethods: that only half works. The thing about prototypes is that they update instances retroactively, whereas duck typing on creation does not. This would matter if, for example, I were to create streams inside Mithril core, before you got a chance to add your methods. Another approach that would technically work is to extend Function.prototype, but that's really ugly.

[nordfjord]

I'd like to offer a rebuttal on your assessment of reduceRight not working.

if we take reduceRight to mean reduce starting from the end of an array, that could be achieved in streams with something like this:

stream.reduceRight = function(fn, initial){
  let collection = []
  let s = stream()
  this.map(d => collection.unshift(d))
  this.onEnd(ev => {
    s(collection.reduce(fn, initial))
    s.end()
  })

  return s
}

It's a bit like saying reduceRight doesn't work if you push into the array afterwards.

[gilbert]

Object.keys(streamMethods).forEach(k => stream[k] = streamMethods[k])

@nordfjord I actually played around with this idea this past weekend. This caused the test suite to run 10 times slower!

One idea would be to have require('mithril/stream-es6') for those who don't mind the es6 environment. Or maybe a require('mithril/stream-slow') for the extendable version? :)

[nordfjord]

@mindeavor yeah, that's kinda expected, Object.keys is slow, I wonder if it would be faster with a for (... in ...) loop.

for (var k in streamMethods) {
  stream[k] = streamMethods[k]
}

Of course we could always end up just using helper methods to extend stream functionality e.g.

function streamFilter(src, fn) {
  var s = m.prop()
  src.map(d => fn(d) ? s(d) : null)
  return s
}

[gilbert]

I didn't mention it, but I was using a for loop. I think having a loop opts-out of a v8 optimization or something.

[lhorie]

It's a bit like saying reduceRight doesn't work if you push into the array afterwards

Hmm, that makes sense. I hadn't thought of it that way.

Object.keys is slow

@mindeavor Do you mean that the reported elapsed was 10 times larger? That doesn't sound right, Object.keys is not that slow... Were you perhaps fiddling with other things as well?

[gilbert]

@lhorie Yeah, when I ran the test suite (util/tests/index.html), changing it to use a for-in loop mixin spiked the runtime from about 2.8s to 25.0s

[gilbert]

Oops, I'm an order of magnitude off :) I just ran it again. It's ~300ms vs ~3000ms.

Here's the diff

        stream.map = map, stream.ap = ap, stream.of = createStream
        stream.valueOf = valueOf, stream.toJSON = toJSON
        stream.run = run, stream.catch = doCatch
+
+       for (var method in module.exports.extensions) {
+               stream[method] = extensions[method]
+       }

        Object.defineProperties(stream, {
...
-module.exports = {stream: createStream, combine: combine, reject: reject, HALT: HALT}
+module.exports = {stream: createStream, combine: combine, reject: reject, HALT: HALT, extensions: {}}

[dead-claudia]

@mindeavor When you use for ... in, always filter with Object.prototype.hasOwnProperty, or you'll see massive perf drops, because engines almost universally optimize for iterating own properties (it's cheap to check just one object), but not for own + inherited properties, which an unfiltered for ... in does. It's bit me a few too many times before, and because of the other inherent gotcha (iterating Object.prototype unintentionally), I always guard for ... in loops unless the surrounding code already never does.

[gilbert]

Ok, so I changed it to:

for (var method in module.exports.extensions) {
  if ( extensions.hasOwnProperty(method) ) {
    stream[method] = extensions[method]
  }
}

but it didn't seem to make a difference.

I did discover something weird, though. If I have my JS console open, it's ~300ms vs ~3000ms. However, if I run the page with my JS console closed, and open it after the tests run, the times then become ~150ms vs ~600ms. Weird...

[dead-claudia]

@mindeavor I'd use Object.keys then (I normally do anyways), but is this really in a perf-sensitive place? Streams are more often used than created, which is why I ask.

[gilbert]

Not sure, I'm just reporting my findings.

[dead-claudia]

@mindeavor I'd say change it just in case. It's maybe an extra line of code or two. (And BTW, I think you meant to use extensions for both).

And FWIW, I pretty much always alias Object.prototype.hasOwnProperty, which is what I was referring to in being somewhat comparable in performance (slower for small objects, faster for larger ones):

var hasOwn = Object.prototype.hasOwnProperty

// later on...
for (var method in extensions) {
  if (hasOwn.call(extensions, method)) {
    stream[method] = extensions[method]
  }
}

What may be slowing your code down is the fact that hasOwn here is known to be Object.prototype.hasOwnProperty as long as it was that way when the script was loaded, but a user could replace extensions.hasOwnProperty and your code breaks. The engine can't avoid checking inherited properties here because of this reason, and engines aren't yet intelligent enough to easily optimize this check away (it's not trivial either, because you have to do IC checks on the full prototype chain to verify this without potentially requiring a ton of memory, and if the property is changed, a full function recompilation is likely in both V8 and SpiderMonkey).

[pygy]

I gave a crack at a faster mixin implementation, using arrays to store names and functions: pygy@b8479f4

[JAForbes]

I'm not against extensible streams. But I am honestly surprised how well just having operators as functions works. E.g. filter(f, stream) instead of stream.filter(f)

Particularly when you do something like

const f = pipe(
  ,filter( x => x < 5)
  ,take(10)
  ,sum
)

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

I'd personally really like for the dust to settle before we try extending streams because I believe we can come up with a way to easily create operators from normal non-stream specific functions efficiently. Some kind of combination of compose + combine so each step doesn't create an intermediary stream.

The fantasy land proposal for lodash is currently the most popular request on their tracker. So I think it would be a shame to have all these mithril specific stream operators when in the future most of us can just take advantage of ramda or lodash to compose our interesting operators in userland. That gives the user more power and reduces the maintenance burden on plugin authors (because they don't need to exist)

We probably need a few more fantasy land primitives to facilitate that. But if I'm right we save ourselves a lot of time, if I'm wrong we can just add the proposed solution but with some experience building real 1.0 apps to aid the design.