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

Does not work well with a few corner cases #8

Closed
gaearon opened this issue Nov 12, 2014 · 14 comments
Closed

Does not work well with a few corner cases #8

gaearon opened this issue Nov 12, 2014 · 14 comments

Comments

@gaearon
Copy link

gaearon commented Nov 12, 2014

I actually like that this strategy enforces async setState because it's more deterministic than default strategy in this aspect. Just like Promises resolve on next tick so there's just one code path and not two.

We've been using this strategy in production with some success but I'm filing this to warn about a few caveats I found:

  • Safari on iOS is really restrictive about playing media in response to clicks. If you have something like <NativeAudio isPlaying> that wraps HTML5 <audio> and toggle isPlaying in response to a click, RAF batching will delay your call until next frame, and iOS Safari will block it. The solution (if you don't want to give up on RAF batching) is to simply call ReactUpdates.flushBatchedUpdates() after setState in click handler.
  • Chrome is very aggressive about not calling animation frames when tab is inactive. This is fine for animations but it looks very weird if some UI update (e.g. AJAX load success) only gets triggered when users switches back to the app. Internally we changed strategy like this:
// Based on https://github.com/petehunt/react-raf-batching
// but also triggers `tick` regularly if tab is inactive.

'use strict';

var ReactUpdates = require('react/lib/ReactUpdates'),
    now = require('react/lib/performanceNow');

var FORCE_TICK_INTERVAL = 1000,
    FORCE_TICK_THRESHOLD = 100,
    lastTick;

function tick() {
  ReactUpdates.flushBatchedUpdates();
  window.requestAnimationFrame(tick);

  lastTick = now();
}

function forceTick() {
  if (now() - lastTick > FORCE_TICK_THRESHOLD) {
    tick();
  }
}

var ReactRAFBatchingStrategy = {
  isBatchingUpdates: true,

  /**
   * Call the provided function in a context within which calls to `setState`
   * and friends are batched such that components aren't updated unnecessarily.
   */
  batchedUpdates: function(callback, a, b) {
    callback(a, b);
  }
};

tick();
setInterval(forceTick, FORCE_TICK_INTERVAL);

module.exports = ReactRAFBatchingStrategy;

Otherwise it works well for us and we never had problems with controlled inputs that some people were reporting.

@steida
Copy link

steida commented Dec 27, 2014

Thank you for report!

@STRML
Copy link

STRML commented Jul 2, 2015

I just wanted to mention, in case anyone uses this solution; there is a major bug that can cause 100% CPU usage in the tab after it has been idling for some time.

The issue is that forceTick() queues another animation frame. This starts another batching loop in parallel with the existing one. Start switching between tabs rapidly on on Chrome and you might start up dozens of parallel RAF loops.

For example, this is how a healthy page ticks:

2015-07-02 13:06:09.790 app.js:31104 tick
2015-07-02 13:06:09.805 app.js:31104 tick
2015-07-02 13:06:09.822 app.js:31104 tick
2015-07-02 13:06:09.865 app.js:31104 tick
2015-07-02 13:06:09.871 app.js:31104 tick
2015-07-02 13:06:09.888 app.js:31104 tick
2015-07-02 13:06:09.907 app.js:31104 tick
2015-07-02 13:06:09.921 app.js:31104 tick

Yet after a very short amount of time, if you switch the tab, this happens:

2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.444 app.js:31104 tick
2015-07-02 13:06:46.459 app.js:31104 tick
2015-07-02 13:06:46.459 app.js:31104 tick
2015-07-02 13:06:46.460 app.js:31104 tick
2015-07-02 13:06:46.460 app.js:31104 tick
2015-07-02 13:06:46.460 app.js:31104 tick
2015-07-02 13:06:46.460 app.js:31104 tick

A fixed version is below.

// Based on https://github.com/petehunt/react-raf-batching
// but also triggers `tick` regularly if tab is inactive.

'use strict';

var ReactUpdates = require('react/lib/ReactUpdates'),
    requestAnimationFrame = require('./RequestAnimationFrame');

var FORCE_TICK_INTERVAL = 1000;

var flush = ReactUpdates.flushBatchedUpdates.bind(ReactUpdates);

var timeout;

function tick() {
  flush();
  requestAnimationFrame(tick);
  clearTimeout(timeout);
  // Do not call `tick()`, that could cause multiple RAF cycles and hog CPU.
  timeout = setTimeout(forceTick, FORCE_TICK_INTERVAL);
}

function forceTick() {
  flush();
  timeout = setTimeout(forceTick, FORCE_TICK_INTERVAL);
}

var ReactRAFBatchingStrategy = {
  isBatchingUpdates: true,

  /**
   * Call the provided function in a context within which calls to `setState`
   * and friends are batched such that components aren't updated unnecessarily.
   */
  batchedUpdates: function(callback) {
    callback.apply(null, Array.prototype.slice.call(arguments, 1));
  },

  inject: function() {
    ReactUpdates.injection.injectBatchingStrategy(ReactRAFBatchingStrategy);
    tick();
  }
};

module.exports = ReactRAFBatchingStrategy;

@gaearon
Copy link
Author

gaearon commented Jul 2, 2015

@STRML Thank you for the explanation!

@STRML
Copy link

STRML commented Jul 2, 2015

FYI it appears I actually caused it again! This is a tricky one to avoid. I've updated the example above which I have verified fixes the problem.

@brandonpapworth
Copy link

You could also try to leverage the pageVisibility API instead of checking if RAF is properly triggering. This would be a much more performant solution. https://developer.mozilla.org/en-US/docs/Web/Guide/User_experience/Using_the_Page_Visibility_API it has been around for a little while.

@petehunt
Copy link
Owner

This project isn't maintained, so I'm closing this issue. React internals have changed enough that this repo probably isn't interesting anymore.

@STRML
Copy link

STRML commented Dec 13, 2015

@petehunt Could you elaborate on that? We are still using RAF batching to pretty good effect when large numbers of updates come down the websocket in succession. Is there something else we should be doing instead?

@petehunt
Copy link
Owner

This was just an npm packaging of something that was in the React repo a while ago that has since been removed: facebook/react#3706

Basically, RAF batching was known to cause some issues with controlled form components, and since Facebook wasn't dogfooding it's likely it wouldn't have gotten fixed.

My suggestion would be to implement a custom batching strategy for your app based on ReactRAFBatchingStrategy.

You could do something like:

"use strict";

var ReactUpdates = require('react/lib/ReactUpdates');

var requestAnimationFrame = require('./requestAnimationFrame');

function tick() {
  if (ReactRAFBatchingStrategy.isBatchingUpdates) {
    ReactUpdates.flushBatchedUpdates();
  }
  ReactRAFBatchingStrategy.isBatchingUpdates = false;
  requestAnimationFrame(tick);
}

var ReactRAFBatchingStrategy = {
  isBatchingUpdates: false,

  /**
   * Call the provided function in a context within which calls to `setState`
   * and friends are batched such that components aren't updated unnecessarily.
   */
  batchedUpdates: function(callback, param) {
    callback(param);
  }
};

requestAnimationFrame(tick);

Then in your websocket event handler, set ReactRAFBatchingStrategy.isBatchingUpdates = true.

Does this make sense?

@STRML
Copy link

STRML commented Dec 13, 2015

Sure - this is what we're using, which ensures updates still fire even if the page isn't visible:

// Based on https://github.com/petehunt/react-raf-batching
// but also triggers `tick` regularly if tab is inactive.

let ReactUpdates = require('react/lib/ReactUpdates');
let requestAnimationFrame = require('./RequestAnimationFrame');

let FORCE_TICK_INTERVAL = 1000;

let flush = ReactUpdates.flushBatchedUpdates.bind(ReactUpdates);

let timeout;

function tick() {
  flush();
  requestAnimationFrame(tick);
  clearTimeout(timeout);
  // Do not call `tick()`, that could cause multiple RAF cycles and hog CPU.
  timeout = setTimeout(forceTick, FORCE_TICK_INTERVAL);
}

function forceTick() {
  flush();
  timeout = setTimeout(forceTick, FORCE_TICK_INTERVAL);
}

let ReactRAFBatchingStrategy = {
  isBatchingUpdates: true,

  /**
   * Call the provided function in a context within which calls to `setState`
   * and friends are batched such that components aren't updated unnecessarily.
   */
  batchedUpdates: function(callback: Function) {
    // Do we have to do this anymore?
    let args = new Array(arguments.length - 1);
    for (let i = 0; i < args.length; ++i) {
      args[i] = arguments[i + 1];
    }
    callback.apply(null, args);
  },

  inject: function() {
    ReactUpdates.injection.injectBatchingStrategy(ReactRAFBatchingStrategy);
    tick();
  }
};

module.exports = ReactRAFBatchingStrategy;

What benefit is there to modifying the isBatchingUpdates flag - do you mean we should be turning it off and on in our WS handlers?

Also, has the arity of batchedUpdates changed (only a single param)?

@petehunt
Copy link
Owner

@STRML setting isBatchingUpdates to true tells React to start buffering. I'm suggesting that you start buffering when a websocket request comes in, and stop buffering when you flush at the next tick (my previous example had a bug which I've corrected). This basically disables RAF batching unless you actually need it because you've received a websocket event.

Bit of a long day today so I'm not sure if I'm making sense... am I? :)

@petehunt
Copy link
Owner

Also, looks it still takes up to 5 parameters, and you should call ReactDOM.unstable_batchedUpdates() instead.

@STRML
Copy link

STRML commented Dec 13, 2015

Appears that ReactDOM.unstable_batchedUpdates() isn't particularly useful (it just calls ReactUpdates.batchedUpdates()); in order to inject a batching strategy, we have to reach into react/lib/ReactUpdates so we can call ReactUpdates.injection.injectBatchingStrategy(). So in any case we're reaching into internals.

@STRML
Copy link

STRML commented Dec 13, 2015

Re: the example, that makes sense to prevent unnecessary RAF ticks - thanks.

@ccorcos
Copy link

ccorcos commented Dec 25, 2015

@petehunt in your example, it looks like ReactRAFBatchingStrategy.isBatchingUpdates = true will only batch for one tick right?

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

6 participants