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

fix(animationFrames): emit the timestamp from the rAF's callback #5438

Merged
merged 7 commits into from Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion api_guard/dist/types/index.d.ts
@@ -1,6 +1,9 @@
export declare const animationFrame: AnimationFrameScheduler;

export declare function animationFrames(timestampProvider?: TimestampProvider): Observable<number>;
export declare function animationFrames(timestampProvider?: TimestampProvider): Observable<{
timestamp: number;
elapsed: number;
}>;

export declare const animationFrameScheduler: AnimationFrameScheduler;

Expand Down
4 changes: 2 additions & 2 deletions spec-dtslint/observables/dom/animationFrames-spec.ts
@@ -1,11 +1,11 @@
import { animationFrames } from 'rxjs';

it('should just be an observable of numbers', () => {
const o$ = animationFrames(); // $ExpectType Observable<number>
const o$ = animationFrames(); // $ExpectType Observable<{ timestamp: number; elapsed: number; }>
});

it('should allow the passing of a timestampProvider', () => {
const o$ = animationFrames(performance); // $ExpectType Observable<number>
const o$ = animationFrames(performance); // $ExpectType Observable<{ timestamp: number; elapsed: number; }>
});

it('should not allow the passing of an invalid timestamp provider', () => {
Expand Down
20 changes: 10 additions & 10 deletions spec/observables/dom/animationFrames-spec.ts
Expand Up @@ -27,9 +27,9 @@ describe('animationFrames', () => {

const result = mapped.pipe(mergeMapTo(animationFrames()));
expectObservable(result, subs).toBe(expected, {
a: ta - tm,
b: tb - tm,
c: tc - tm,
a: { elapsed: ta - tm, timestamp: ta },
b: { elapsed: tb - tm, timestamp: tb },
c: { elapsed: tc - tm, timestamp: tc },
});
});
});
Expand All @@ -50,9 +50,9 @@ describe('animationFrames', () => {

const result = mapped.pipe(mergeMapTo(animationFrames(timestampProvider)));
expectObservable(result, subs).toBe(expected, {
a: 50,
b: 150,
c: 250,
a: { elapsed: 50, timestamp: 100 },
b: { elapsed: 150, timestamp: 200 },
c: { elapsed: 250, timestamp: 300 },
});
});
});
Expand All @@ -71,8 +71,8 @@ describe('animationFrames', () => {

const result = mapped.pipe(mergeMapTo(animationFrames().pipe(take(2))));
expectObservable(result).toBe(expected, {
a: ta - tm,
b: tb - tm,
a: { elapsed: ta - tm, timestamp: ta },
b: { elapsed: tb - tm, timestamp: tb },
});

testScheduler.flush();
Expand All @@ -98,8 +98,8 @@ describe('animationFrames', () => {

const result = mapped.pipe(mergeMapTo(animationFrames().pipe(takeUntil(signal))));
expectObservable(result).toBe(expected, {
a: ta - tm,
b: tb - tm,
a: { elapsed: ta - tm, timestamp: ta },
b: { elapsed: tb - tm, timestamp: tb },
});

testScheduler.flush();
Expand Down
47 changes: 33 additions & 14 deletions src/internal/observable/dom/animationFrames.ts
@@ -1,14 +1,14 @@
import { Observable } from '../../Observable';
import { Subscription } from '../../Subscription';
import { TimestampProvider } from "../../types";
import { dateTimestampProvider } from '../../scheduler/dateTimestampProvider';
import { performanceTimestampProvider } from '../../scheduler/performanceTimestampProvider';
import { requestAnimationFrameProvider } from '../../scheduler/requestAnimationFrameProvider';

/**
* An observable of animation frames
*
* Emits the the amount of time elapsed since subscription on each animation frame. Defaults to elapsed
* milliseconds. Does not end on its own.
* Emits the the amount of time elapsed since subscription and the timestamp on each animation frame.
* Defaults to milliseconds provided to the requestAnimationFrame's callback. Does not end on its own.
*
* Every subscription will start a separate animation loop. Since animation frames are always scheduled
* by the browser to occur directly before a repaint, scheduling more than one animation frame synchronously
Expand All @@ -31,7 +31,7 @@ import { requestAnimationFrameProvider } from '../../scheduler/requestAnimationF
* const diff = end - start;
* return animationFrames().pipe(
* // Figure out what percentage of time has passed
* map(elapsed => elapsed / duration),
* map(({elapsed}) => elapsed / duration),
* // Take the vector while less than 100%
* takeWhile(v => v < 1),
* // Finish with 100%
Expand Down Expand Up @@ -71,26 +71,45 @@ import { requestAnimationFrameProvider } from '../../scheduler/requestAnimationF
* const source$ = animationFrames(customTSProvider);
*
* // Log increasing numbers 0...1...2... on every animation frame.
* source$.subscribe(x => console.log(x));
* source$.subscribe(({ elapsed }) => console.log(elapsed));
* ```
*
* @param timestampProvider An object with a `now` method that provides a numeric timestamp
*/
export function animationFrames(timestampProvider: TimestampProvider = dateTimestampProvider) {
return timestampProvider === dateTimestampProvider ? DEFAULT_ANIMATION_FRAMES : animationFramesFactory(timestampProvider);
export function animationFrames(timestampProvider?: TimestampProvider) {
return timestampProvider ? animationFramesFactory(timestampProvider) : DEFAULT_ANIMATION_FRAMES;
}

/**
* Does the work of creating the observable for `animationFrames`.
* @param timestampProvider The timestamp provider to use to create the observable
*/
function animationFramesFactory(timestampProvider: TimestampProvider) {
function animationFramesFactory(timestampProvider?: TimestampProvider) {
const { schedule } = requestAnimationFrameProvider;
return new Observable<number>(subscriber => {
const start = timestampProvider.now();
return new Observable<{ timestamp: number, elapsed: number }>(subscriber => {
let subscription: Subscription;
const run = () => {
subscriber.next(timestampProvider.now() - start);
// If no timestamp provider is specified, use performance.now() - as it
// will return timestamps 'compatible' with those passed to the run
// callback and won't be affected by NTP adjustments, etc.
const provider = timestampProvider || performanceTimestampProvider;
// Capture the start time upon subscription, as the run callback can remain
// queued for a considerable period of time and the elapsed time should
// represent the time elapsed since subscription - not the time since the
// first rendered animation frame.
const start = provider.now();
const run = (timestamp: DOMHighResTimeStamp | number) => {
Copy link
Member

Choose a reason for hiding this comment

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

Note that this still doesn't really solve the problem that this is untestable in TestScheduler run mode, which automatically patches our schedulers.

Copy link
Member

Choose a reason for hiding this comment

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

I think the solution here is going to be creating a lightweight wrapper around rAF, such that we can override it in run mode. That's what the animationFrameScheduler is supposed to be, but it's inadequate, because it doesn't provide the timestamp, and our schedulers in general are perhaps a bit overkill for this use case.

Something like this should suffice:

export const rAF = {
   schedule(callback: (timestamp: number) => void): Subscription {
     const id = this.impl(callback);
     return new Subscription(() => cancelAnimationFrame(id));
   },
   impl: requestAnimationFrame,
}

Then, in our run mode, we just need to pull that in and patch the impl to override it's behavior where we are using it.

We could also just monkey patch rAF directly, but that has other implications I'm not keen on.

We also need some sort of way to declare when animation frames should be fired in a test. Probably another, idempotent helper that we add to the context passed to TestScheduler.run. like:

rxTest.run(({ cold, expectObservable, animationFrames }) => {
  animationFrames('-----x----x----x----x---x-----');
  // rest of the test here
});

// Use the provider's timestamp to calculate the elapsed time. Note that
// this means - if the caller hasn't passed a provider - that
// performance.now() will be used instead of the timestamp that was
// passed to the run callback. The reason for this is that the timestamp
// passed to the callback can be earlier than the start time, as it
// represents the time at which the browser decided it would render any
// queued frames - and that time can be earlier the captured start time.
const now = provider.now();
subscriber.next({
timestamp: timestampProvider ? now : timestamp,
benlesh marked this conversation as resolved.
Show resolved Hide resolved
elapsed: now - start
});
if (!subscriber.closed) {
subscription = schedule(run);
}
Expand All @@ -103,7 +122,7 @@ function animationFramesFactory(timestampProvider: TimestampProvider) {
}

/**
* In the common case, where `Date` is passed to `animationFrames` as the default,
* In the common case, where the timestamp provided by the rAF API is used,
* we use this shared observable to reduce overhead.
*/
const DEFAULT_ANIMATION_FRAMES = animationFramesFactory(dateTimestampProvider);
const DEFAULT_ANIMATION_FRAMES = animationFramesFactory();