Skip to content

Commit

Permalink
Animated: Defer onAnimatedValueUpdate on Attach + Native (#48715)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #48715

{D68154908} fixed a problem with the `onAnimatedValueUpdate` listener not being correctly attached if `__attach` were called before `__makeNative` (which sets `__isNative` to true).

We're potentially seeing production symptoms of stuttering interactions and user responsiveness, after queuing up many operations. Our hypothesis is that in scenarios where `ensureUpdateSubscriptionExists` is being called during `__makeNative` (instead of during `__attach`), a backup of operations occurs leading to these symptoms.

This diff attempts to validate and mitigate this hypothesis by deferring `ensureUpdateSubscriptionExists` to when an `AnimatedValue` instance has had both `__attach` and `__makeNative` invoked.

Changelog:
[Internal]

Differential Revision: D68236594

fbshipit-source-id: 2089100a773ebfc161fb5b567123eb58a893939f
  • Loading branch information
yungsters authored and fabriziocucci committed Jan 20, 2025
1 parent bb5f971 commit 3abb4ca
Showing 1 changed file with 16 additions and 8 deletions.
24 changes: 16 additions & 8 deletions packages/react-native/Libraries/Animated/nodes/AnimatedValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ function _executeAsAnimatedBatch(id: string, operation: () => void) {
* See https://reactnative.dev/docs/animatedvalue
*/
export default class AnimatedValue extends AnimatedWithChildren {
#attached: boolean = false;
#updateSubscription: ?EventSubscription = null;

_value: number;
Expand All @@ -107,14 +108,8 @@ export default class AnimatedValue extends AnimatedWithChildren {
}

__attach(): void {
if (this.__isNative) {
// NOTE: In theory, we should only need to call this when any listeners
// are added. However, there is a global `onUserDrivenAnimationEnded`
// listener that relies on `onAnimatedValueUpdate` having fired to update
// the values in JavaScript. If that listener is removed, this could be
// re-optimized.
this.#ensureUpdateSubscriptionExists();
}
this.#attached = true;
this.#ensureUpdateSubscriptionExists();
}

__detach(): void {
Expand All @@ -126,6 +121,7 @@ export default class AnimatedValue extends AnimatedWithChildren {
}
this.stopAnimation();
super.__detach();
this.#attached = false;
}

__getValue(): number {
Expand All @@ -137,10 +133,22 @@ export default class AnimatedValue extends AnimatedWithChildren {
this.#ensureUpdateSubscriptionExists();
}

/**
* NOTE: In theory, we should only need to call this when any listeners
* are added. However, there is a global `onUserDrivenAnimationEnded`
* listener that relies on `onAnimatedValueUpdate` having fired to update
* the values in JavaScript. If that listener is removed, this could be
* re-optimized.
*/
#ensureUpdateSubscriptionExists(): void {
if (this.#updateSubscription != null) {
return;
}
// The order in which `__attach` and `__makeNative` are called is not
// deterministic, and we only want to do this when both have occurred.
if (!this.#attached || !this.__isNative) {
return;
}
const nativeTag = this.__getNativeTag();
NativeAnimatedAPI.startListeningToAnimatedNodeValue(nativeTag);
const subscription: EventSubscription =
Expand Down

0 comments on commit 3abb4ca

Please sign in to comment.