-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
19a7fc3
to
2cf4c0a
Compare
2cf4c0a
to
5eb2ca7
Compare
- Also adds tests and test harness for requestAnimationFrame stubbing with sinon - Updates TypeScript lib to use ES2018 (so we can use `findIndex` on `Array`).
5eb2ca7
to
36f1116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is variously referred to as the run
loop and the animate
loop. Maybe pick one term for it and use that term only?
I think this is a good idea to add to the core library. Anyway, I'd vote for an import site |
Somewhat related to this conversation, I've published a package ( 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} />
)
}
}
} |
@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 |
Actually, I'm on the fence about whether or not to have each one start it's own animation loop. If you call 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 |
Now each subscription will cause a new animation frame loop to be kicked off, instead of trying to share a single animation loop.
af9ead1
to
8d24153
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Regarding the import location, I'd vote for keeping it as-is - Moving to an |
79f8f0b
to
8d24153
Compare
FWIW I came here to request this behavior, glad to see it! |
findIndex
onArray
).This is a proposed API, we haven't decided on anything yet
Example 1: Just a simple use.
Example 2: Tweening animation function
See documentation in code for more information.
Highlights:
range(0, animationFrameScheduler)
orinterval(0, animationFrameScheduler)
TBD:
rxjs
, or do we add a new import site forrxjs/animation
? If the latter, does it need to bedom-animation
?Observable<void>
preferrable, and make users compose to get the elapsed time?