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

feat(animationFrames): Adds an observable of animationFrames #5021

Merged
merged 2 commits into from Sep 18, 2019

Conversation

@benlesh
Copy link
Member

commented Sep 16, 2019

  • Also adds tests and test harness for requestAnimationFrame stubbing with sinon
  • Updates TypeScript lib to use ES2018 (so we can use findIndex on Array).

This is a proposed API, we haven't decided on anything yet

Example 1: Just a simple use.

import { animationFrames } from 'rxjs';

// Log the elapsed milliseconds at each animation frame.
animationFrames().subscribe(elapsed => console.log(elapsed));

Example 2: Tweening animation function

 import { animationFrames } from 'rxjs';
 import { map, takeWhile, endWith } from 'rxjs/operators';
 
 function tween(start: number, end: number, duration: number) {
   const diff = end - start;
   return animationFrames().pipe(
     // Figure out what percentage of time has passed
     map(elapsed => elapsed / duration),
     // Take the vector while less than 100%
     takeWhile(v => v < 1),
     // Finish with 100%
     endWith(1),
     // Calculate the distance traveled between start and end
     map(v => v * diff + start)
   );
 }
 
 // Setup a div for us to move around
 const div = document.createElement('div');
 document.body.appendChild(div);
 div.style.position = 'absolute';
 div.style.width = '40px';
 div.style.height = '40px';
 div.style.backgroundColor = 'lime';
 div.style.transform = 'translate3d(10px, 0, 0)';
 
 tween(10, 200, 4000).subscribe(x => {
   div.style.transform = `translate3d(${x}px, 0, 0)`;
 });

See documentation in code for more information.

Highlights:

  • Starts a single animation loop for N subscriptions.
  • If no more subscribers are subscribed to the animation loop, it will terminate the animation loop.
  • Replacement for odd patterns like range(0, animationFrameScheduler) or interval(0, animationFrameScheduler)
  • Outputs more useful values for animations than above recipes.

TBD:

  • Do we want this at all?
  • Do we want to export this from rxjs, or do we add a new import site for rxjs/animation? If the latter, does it need to be dom-animation?
  • Recommendations for testing usage of this to be included in docs?
  • Do we find this approach agreeable?
    • Is Observable<void> preferrable, and make users compose to get the elapsed time?
    • Do we keep it as-is?
    • Do we want each subscription to this to start a different animation loop? (Seems inefficient)
@benlesh benlesh force-pushed the benlesh:animationFrame branch 2 times, most recently from 19a7fc3 to 2cf4c0a Sep 16, 2019
@benlesh benlesh requested review from cartant and kwonoj Sep 16, 2019
@benlesh benlesh force-pushed the benlesh:animationFrame branch from 2cf4c0a to 5eb2ca7 Sep 16, 2019
- Also adds tests and test harness for requestAnimationFrame stubbing with sinon
- Updates TypeScript lib to use ES2018 (so we can use `findIndex` on `Array`).
@benlesh benlesh force-pushed the benlesh:animationFrame branch from 5eb2ca7 to 36f1116 Sep 16, 2019
Copy link
Collaborator

left a comment

This LGTM. Just a nit about naming. I'm also wondering if the comments - for the run/animate loop - should include some justification for the complexity of managing an array of subscribers together with a single requestAnimationFrame loop (rather than having a loop per subscription). Presumably, this is done to avoid lots of requestAnimationFrame calls per frame when there are lots of subscribers?

* Executes notification of all subscribers, then reschedules itself.
* Do not call directly. This is the "run loop".
*/
function animate() {

This comment has been minimized.

Copy link
@cartant

cartant Sep 17, 2019

Collaborator

This is variously referred to as the run loop and the animate loop. Maybe pick one term for it and use that term only?

@JWO719

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

I think this is a good idea to add to the core library. Anyway, I'd vote for an import site rxjs/dom, because this would fit quite well with #5002 & we used to have rxjs-dom, but didn't migrate it.

@basham

This comment has been minimized.

Copy link

commented Sep 17, 2019

Somewhat related to this conversation, I've published a package (conduit-rxjs-react) that connects RxJS observables to React components. As an optimization, I limit the number of state updates using requestAnimationFrame, bindCallback, and audit to reduce thrashing. If there are multiple state updates while waiting for the animation frame callback, only the most recent update is kept. The following is an abbreviated version of the code to demonstrate this implementation. Maybe this could be rewritten slightly to take advantage of an animationFrames observable.

import { Component } from 'react'
import { bindCallback } from 'rxjs'
import { audit } from 'rxjs/operators'

function connect (WrappedComponent, state$) {
  return class StreamComponent extends Component {
    constructor () {
      state$.pipe(
        audit(bindCallback((x, callback) =>
          window.requestAnimationFrame(callback)
        ))
      ).subscribe((state) => {
        this.setState(state)
      })
    }
    render () {
      return (
        <WrappedComponent {...this.state} />
      )
    }
  }
}
@benlesh

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

Presumably, this is done to avoid lots of requestAnimationFrame calls per frame when there are lots of subscribers?

@cartant That's exactly right. There's no reason to schedule multiple animation frames, when sharing one is more efficient, at least in the common case. IMO, it's also surpising and confusing for developers that they can call requestAnimationFrame(updateBox) and requestAnimationFrame(updateBall), and the box will be updated at a separate time than the ball. I guess the method isn't called scheduleForNextAnimationFrame(fn), but I know I found it surprising the callbacks didn't land at the same time.

@benlesh

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

Actually, I'm on the fence about whether or not to have each one start it's own animation loop.

If you call requestAnimationFrame 3 times in a row, synchronously, it will indeed fire 3 separate tasks, but they also all occur directly before the repaint. So having them all in loop probably isn't as big of a win as I think it is.

I think I'm going to make each one start it's own loop, and if users want to share that loop they can compose that behavior with share.

Now each subscription will cause a new animation frame loop to be kicked off, instead of trying to share a single animation loop.
@benlesh benlesh force-pushed the benlesh:animationFrame branch from af9ead1 to 8d24153 Sep 17, 2019
@benlesh

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

Okay, the change is made, and I like this better. It now creates a different animation loop per subscriber, and if a developer wants to share that animation loop, they just use share, although i'm not at all sure anymore that it's a big win.

Copy link
Collaborator

left a comment

LGTM

@cartant

This comment has been minimized.

Copy link
Collaborator

commented Sep 17, 2019

Regarding the import location, I'd vote for keeping it as-is - import { animationFrames } from "rxjs" - for now, as that's import location for animationFrameScheduler.

Moving to an "rxjs/dom" import location could be something that we could look at later - as part of a restructuring. Doing so would allow us to do some tricky things with TypeScript's dom library typings so that fromEvent could be 'smart' and correctly infer the event object that's associated with mouse events, etc.

@benlesh benlesh force-pushed the benlesh:animationFrame branch from 79f8f0b to 8d24153 Sep 18, 2019
@benlesh benlesh merged commit 6a4cd68 into ReactiveX:master Sep 18, 2019
5 checks passed
5 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: dtslint Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: typescript3 Your tests passed on CircleCI!
Details
@jayphelps

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

It now creates a different animation loop per subscriber, and if a developer wants to share that animation loop, they just use share, although i'm not at all sure anymore that it's a big win.

FWIW I came here to request this behavior, glad to see it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.