From 9c3e6ae9f0f246e5fd8cdc3599801c37d1c11690 Mon Sep 17 00:00:00 2001 From: Spencer Ahrens Date: Fri, 17 Mar 2017 22:02:04 -0700 Subject: [PATCH] Fix Animated.event attach/detach on component re-render Summary: Need to make sure `detach` happens on the old `scrollableNode` before it's unmounted and that `attach` happens on the new `scrollableNode` after it's mounted. This should also be more performant because the detach step no longer requires iterating through all the props, most of which are not animated, and we filter out unneeded updates if props or ref haven't changed. = Test Plan = Hook up native onscroll events in `FlatListExample` and toggle "debug" - before, the events would stop working because they would try to attach to the old unmounted node, but with this diff it keeps working as expected. Reviewed By: fkgozali Differential Revision: D4687186 fbshipit-source-id: 313a7964d4614190308490a51fc4f56abb6690f8 --- .../Animated/src/AnimatedImplementation.js | 74 +++++++++---------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/Libraries/Animated/src/AnimatedImplementation.js b/Libraries/Animated/src/AnimatedImplementation.js index 769b3b0164a64e..09c3c2129452a6 100644 --- a/Libraries/Animated/src/AnimatedImplementation.js +++ b/Libraries/Animated/src/AnimatedImplementation.js @@ -1510,9 +1510,9 @@ class AnimatedStyle extends AnimatedWithChildren { // Recursively get values for nested styles (like iOS's shadowOffset) __walkStyleAndGetValues(style) { - let updatedStyle = {}; - for (let key in style) { - let value = style[key]; + const updatedStyle = {}; + for (const key in style) { + const value = style[key]; if (value instanceof Animated) { if (!value.__isNative) { // We cannot use value of natively driven nodes this way as the value we have access from @@ -1535,9 +1535,9 @@ class AnimatedStyle extends AnimatedWithChildren { // Recursively get animated values for nested styles (like iOS's shadowOffset) __walkStyleAndGetAnimatedValues(style) { - let updatedStyle = {}; - for (let key in style) { - let value = style[key]; + const updatedStyle = {}; + for (const key in style) { + const value = style[key]; if (value instanceof Animated) { updatedStyle[key] = value.__getAnimatedValue(); } else if (value && !Array.isArray(value) && typeof value === 'object') { @@ -1690,7 +1690,9 @@ class AnimatedProps extends Animated { } setNativeView(animatedView: any): void { - invariant(this._animatedView === undefined, 'Animated view already set.'); + if (this._animatedView === animatedView) { + return; + } this._animatedView = animatedView; if (this.__isNative) { this.__connectAnimatedView(); @@ -1729,7 +1731,9 @@ class AnimatedProps extends Animated { function createAnimatedComponent(Component: any): any { class AnimatedComponent extends React.Component { _component: any; + _prevComponent: any; _propsAnimated: AnimatedProps; + _eventDetachers: Array = []; _setComponentRef: Function; constructor(props: Object) { @@ -1739,7 +1743,7 @@ function createAnimatedComponent(Component: any): any { componentWillUnmount() { this._propsAnimated && this._propsAnimated.__detach(); - this._detachNativeEvents(this.props); + this._detachNativeEvents(); } setNativeProps(props) { @@ -1752,42 +1756,28 @@ function createAnimatedComponent(Component: any): any { componentDidMount() { this._propsAnimated.setNativeView(this._component); - - this._attachNativeEvents(this.props); + this._attachNativeEvents(); } - _attachNativeEvents(newProps) { - if (newProps !== this.props) { - this._detachNativeEvents(this.props); - } - + _attachNativeEvents() { // Make sure to get the scrollable node for components that implement // `ScrollResponder.Mixin`. - const ref = this._component.getScrollableNode ? + const scrollableNode = this._component.getScrollableNode ? this._component.getScrollableNode() : this._component; - for (const key in newProps) { - const prop = newProps[key]; + for (const key in this.props) { + const prop = this.props[key]; if (prop instanceof AnimatedEvent && prop.__isNative) { - prop.__attach(ref, key); + prop.__attach(scrollableNode, key); + this._eventDetachers.push(() => prop.__detach(scrollableNode, key)); } } } - _detachNativeEvents(props) { - // Make sure to get the scrollable node for components that implement - // `ScrollResponder.Mixin`. - const ref = this._component.getScrollableNode ? - this._component.getScrollableNode() : - this._component; - - for (const key in props) { - const prop = props[key]; - if (prop instanceof AnimatedEvent && prop.__isNative) { - prop.__detach(ref, key); - } - } + _detachNativeEvents() { + this._eventDetachers.forEach(remove => remove()); + this._eventDetachers = []; } _attachProps(nextProps) { @@ -1820,10 +1810,6 @@ function createAnimatedComponent(Component: any): any { callback, ); - if (this._component) { - this._propsAnimated.setNativeView(this._component); - } - // When you call detach, it removes the element from the parent list // of children. If it goes to 0, then the parent also detaches itself // and so on. @@ -1835,9 +1821,18 @@ function createAnimatedComponent(Component: any): any { oldPropsAnimated && oldPropsAnimated.__detach(); } - componentWillReceiveProps(nextProps) { - this._attachProps(nextProps); - this._attachNativeEvents(nextProps); + componentWillReceiveProps(newProps) { + this._attachProps(newProps); + } + + componentDidUpdate(prevProps) { + if (this._component !== this._prevComponent) { + this._propsAnimated.setNativeView(this._component); + } + if (this._component !== this._prevComponent || prevProps !== this.props) { + this._detachNativeEvents(); + this._attachNativeEvents(); + } } render() { @@ -1850,6 +1845,7 @@ function createAnimatedComponent(Component: any): any { } _setComponentRef(c) { + this._prevComponent = this._component; this._component = c; }