Permalink
Browse files

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
  • Loading branch information...
sahrens authored and facebook-github-bot committed Mar 18, 2017
1 parent f48b54b commit 9c3e6ae9f0f246e5fd8cdc3599801c37d1c11690
Showing with 35 additions and 39 deletions.
  1. +35 −39 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<Function> = [];
_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;
}

2 comments on commit 9c3e6ae

@oblador

This comment has been minimized.

Show comment
Hide comment
@oblador

oblador Mar 18, 2017

Contributor

👏

Contributor

oblador replied Mar 18, 2017

👏

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Mar 18, 2017

Collaborator

Nice fix!

Collaborator

janicduplessis replied Mar 18, 2017

Nice fix!

Please sign in to comment.