Skip to content

Commit

Permalink
Only use value of natively driven nodes on initial render
Browse files Browse the repository at this point in the history
Summary:
D36612758 (40f4c66) attempted to remove the check that the animated node was native when fetching animated prop values for render, in order to fix an issue that arises when a node is converted to native before the initial call to render to mount the component, where the initial value is not applied.

However, this causes issues for cases where we call setValue on an animated node soon after render, such as on componentDidUpdate. setValue first updates the animated node value on JS side, then calls the native API setAnimatedNodeValue. The problem is that the next render will then use these updated values in the style props (because we removed the check for native in D36612758 (40f4c66)), resulting in a diff in props. Since Animated and Fabric aren't synchronized, there's a race condition between SurfaceMountingManager.updateProps and NativeAnimatedNodesManager.runUpdates

To allow proper rendering of initial values of native animated nodes, and mitigate the issue when calling setValue on an animated node soon after render, during initial render we use the initial values of both native and non-native driven nodes, and on subsequent renders we only use the initial values of non-native driven nodes.

An alternative considered was to add internal state to the nodes themselves (AnimatedProps, AnimatedStyle), but keeping it in sync with the component state is not easy/clean as AnimatedProps can be recreated and reattached at any time for a mounted component.

Changelog:
[Internal][Fixed] - Only use value of natively driven nodes on initial render

Reviewed By: JoshuaGross

Differential Revision: D36902220

fbshipit-source-id: c20f711aa97d18a2ed549b5f90c6296bf19bb327
  • Loading branch information
genkikondo authored and facebook-github-bot committed Jun 3, 2022
1 parent 11141b8 commit a041951
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 8 deletions.
5 changes: 4 additions & 1 deletion Libraries/Animated/createAnimatedComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
_prevComponent: any;
_propsAnimated: AnimatedProps;
_eventDetachers: Array<Function> = [];
_isInitialRender: boolean = true;

// Only to be used in this file, and only in Fabric.
_animatedComponentId: string = `${animatedComponentNextId++}:animatedComponent`;
Expand Down Expand Up @@ -200,7 +201,8 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
});

render() {
const {style = {}, ...props} = this._propsAnimated.__getValue() || {};
const {style = {}, ...props} =
this._propsAnimated.__getValue(this._isInitialRender) || {};
const {style: passthruStyle = {}, ...passthruProps} =
this.props.passthroughAnimatedPropExplicitValues || {};
const mergedStyle = {...style, ...passthruStyle};
Expand Down Expand Up @@ -232,6 +234,7 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
this._propsAnimated.setNativeView(this._component);
this._attachNativeEvents();
this._markUpdateComplete();
this._isInitialRender = false;
}

UNSAFE_componentWillReceiveProps(newProps: any) {
Expand Down
11 changes: 9 additions & 2 deletions Libraries/Animated/nodes/AnimatedProps.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,19 @@ class AnimatedProps extends AnimatedNode {
this._callback = callback;
}

__getValue(): Object {
__getValue(isInitialRender: boolean = true): Object {
const props: {[string]: any | ((...args: any) => void)} = {};
for (const key in this._props) {
const value = this._props[key];
if (value instanceof AnimatedNode) {
props[key] = value.__getValue();
// During initial render we want to use the initial value of both natively and non-natively
// driven nodes. On subsequent renders, we cannot use the value of natively driven nodes
// as they may not be up to date.
if (value instanceof AnimatedStyle) {
props[key] = value.__getValue(isInitialRender);
} else if (isInitialRender || !value.__isNative) {
props[key] = value.__getValue();
}
} else if (value instanceof AnimatedEvent) {
props[key] = value.__getHandler();
} else {
Expand Down
15 changes: 10 additions & 5 deletions Libraries/Animated/nodes/AnimatedStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,29 @@ class AnimatedStyle extends AnimatedWithChildren {
}

// Recursively get values for nested styles (like iOS's shadowOffset)
_walkStyleAndGetValues(style: any) {
_walkStyleAndGetValues(style: any, isInitialRender: boolean) {
const updatedStyle: {[string]: any | {...}} = {};
for (const key in style) {
const value = style[key];
if (value instanceof AnimatedNode) {
updatedStyle[key] = value.__getValue();
// During initial render we want to use the initial value of both natively and non-natively
// driven nodes. On subsequent renders, we cannot use the value of natively driven nodes
// as they may not be up to date.
if (isInitialRender || !value.__isNative) {
updatedStyle[key] = value.__getValue();
}
} else if (value && !Array.isArray(value) && typeof value === 'object') {
// Support animating nested values (for example: shadowOffset.height)
updatedStyle[key] = this._walkStyleAndGetValues(value);
updatedStyle[key] = this._walkStyleAndGetValues(value, isInitialRender);
} else {
updatedStyle[key] = value;
}
}
return updatedStyle;
}

__getValue(): Object {
return this._walkStyleAndGetValues(this._style);
__getValue(isInitialRender: boolean = true): Object {
return this._walkStyleAndGetValues(this._style, isInitialRender);
}

// Recursively get animated values for nested styles (like iOS's shadowOffset)
Expand Down

0 comments on commit a041951

Please sign in to comment.