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

throttleWhen #78

Merged
merged 3 commits into from
Mar 11, 2018
Merged

throttleWhen #78

merged 3 commits into from
Mar 11, 2018

Conversation

ccorcos
Copy link
Contributor

@ccorcos ccorcos commented Dec 26, 2015

So I'm not sure this is the best name for this function, and its pretty hard to explain without an example.

My motivation (and primary use case) is this:

Given the typical functional UI init-update-view pattern, I wanted a way to actions and their updates without having to re-render on each execution. So rather than this:

// Streams
const actions$ = stream(); // All modifications to the state originate here
const model$ = flyd.scan(update, 0, actions$); // Contains the entire state of the application
const vnode$ = flyd.map(view(actions$), model$); // Stream of virtual nodes to render

// Begin rendering when the DOM is ready
window.addEventListener('DOMContentLoaded', () => {
  const container = document.getElementById('container');
  flyd.scan(patch, container, vnode$);
});

We can have this:

// Streams
const actions$ = stream(); // All modifications to the state originate here
const model$ = flyd.scan(update, 0, actions$); // Contains the entire state of the application
const throttle$ = stream(false) // throttle rendering when we have a bunch of updates coming in
const vnode$ = flyd.map(view(actions$), throttleWhen(throttle$, model$)); // Stream of virtual nodes to render

// Begin rendering when the DOM is ready
window.addEventListener('DOMContentLoaded', () => {
  const container = document.getElementById('container');
  flyd.scan(patch, container, vnode$);
});

So now, if we have a bunch of updates all happening at once, we can make sure not to compute too much:

throttle$(true)
aBunchOfActions.map(action$)
throttle$(false)

Let me know what you think! I copied the format of the other modules. I'm more than happy to change the name if something else makes more sense.

  • batchWhen makes sense except I'm not accumulating the results blocked by the throttle$. It may make sense to make batchWhen first and compose it into this function though...
  • dropWhen makes sense except we want to emit the latest dropped value once we resume.
  • throttleWhen makes sense except throttling tends to be associated with time, at least in ReactiveX.

Then explanation is just terrible. I'm not sure how to explain this in a concise way without the example...

@ccorcos
Copy link
Contributor Author

ccorcos commented Dec 26, 2015

How about this implementation instead:

import flyd from 'flyd'
import { dropRepeats } from 'flyd/module/droprepeats'

const batchWhen = flyd.curryN(2, (sBool, sA) => {
  let batch = []

  const ns = flyd.combine(function(sBool, sA, self, changed) {

    const sBoolChanged = contains(sBool, changed)
    const sAChanged = contains(sA, changed)

    if (sBoolChanged) {
      if (sAChanged) {
        if (sBool()) {
          // if Bool and A change and were batching then
          // push to the batch
          batch.push(sA())
        } else {
          // if Bool and A change and we're not batching
          // anymore, then push the batch
          batch.push(sA())
          self(batch)
          batch = []
        }
      } else {
        if (!sBool()) {
          // if Bool changed but A didnt then push the batch
          // if there were any batching
          if (batch.length > 0) {
            self(batch)
            batch = []
          }
        }
      }
    } else if (sAChanged) {
      if (sBool()) {
        // if we're batching then push to the batch
        batch.push(sA())
      } else {
        // otherwise send it alone
        self([sA()])
      }
    }

  }, [dropRepeats(sBool), sA])

  return ns
});

export default batchWhen
import flyd from 'flyd'
import batchWhen from 'elmish/utils/batchWhen'
import last from 'ramda/src/last'

const throttleWhen = flyd.curryN(2, (sBool, sA) => {
  return flyd.map(last, batchWhen(sBool, sA))
});

export default throttleWhen

@paldepind
Copy link
Owner

I can certainly see the use for this.

I think a stream created by batchWhen should emit arrays of values. It wants to emit multiple values at the same time.

Does this implementation do what you want?

var flyd = require('../../lib');
var dropRepeats = require('../droprepeats').dropRepeats;

// Stream bool -> Stream a -> Stream a
module.exports = flyd.curryN(2, function(sBool, sA) {
  var throttledA = flyd.combine(function(sBool, sA, self) {
    if (sBool() == false) {
      self(sA())
    }
  }, [dropRepeats(sBool), sA]);
});

@ccorcos
Copy link
Contributor Author

ccorcos commented Dec 27, 2015

Yeah, I think batchWhen is far more useful and throttleWhen is just an easy map over the last if you want to. Your implementation is really close to throttleWhen but doesn't quite work if you toggle sBool without every adding an event to sA

@paldepind paldepind mentioned this pull request Jan 23, 2016
@paldepind
Copy link
Owner

Hi, @ccorcos.

I'm sorry, but I totally forgot about this PR.

You're right that my implementation does not work correctly when sBool.

What is the difference between this and keepWhen? It batches the values into an array while the blocking stream blocks?

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 26, 2016

When the blocking stream blocks, we batch all the values into an array. I think batchWhen is really the function I want. throttleWhen is a simple mapping over that.

@paldepind
Copy link
Owner

Ok. I understand now. The last implementation you posted of batchWhen looks great to me. Very simple and straightforward.

If you update the PR I'll happily merge it 😄

@ccorcos
Copy link
Contributor Author

ccorcos commented Feb 9, 2016

hey would you mind checking out that one test? I'm very confused why it fails...

the first stream is true, it will send a batch of all events from the first
stream that have occured while the first stream was true. If an event from
the second stream occurs while the first stream is false, it will be dispatched
as a single element batch.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is a bit hard to understand. Is the second sentence about when the first stream changes to true?

@paldepind
Copy link
Owner

I've found the error. A very tricky one indeed. I started by looking at all the wrong places :)

You'll have to add semicolons for the style check to be happy.

@ccorcos
Copy link
Contributor Author

ccorcos commented Feb 15, 2016

Weird that JS doesnt throw and error and instead just replaces with undefined. lol.

Also, I wasnt getting the eslint errors, but I added the semicolons i could find anyways.

@StreetStrider
Copy link
Contributor

@ccorcos, why it should throw? [1, 2][3] is syntactically correct, you just want to get 3rd element of array with two elements :) JS is permissive when you pick by nonexistent key.

@ccorcos
Copy link
Contributor Author

ccorcos commented Feb 15, 2016

@StreetStrider lol, you're right! I wasn't even thinking about indexing. I suppose it would be an error if it was [3] [1,2]

@MatthewCallis
Copy link

I was scanning the API for this exact method! Is there anything preventing this from being merged?

@paldepind
Copy link
Owner

@MatthewCallis No. It looks good to me. It just slipped through the cracks and I forgot about it 😭 Thanks a lot @ccorcos. I'll merge it.

@paldepind paldepind merged commit 34f59e1 into paldepind:master Mar 11, 2018
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

Successfully merging this pull request may close these issues.

None yet

4 participants