Skip to content

Commit

Permalink
Fix setValue to work properly on natively driven nodes
Browse files Browse the repository at this point in the history
Summary:
This change fixes an issue with calling `setValue` for natively driven nodes. As of now an attempt to call this method from JS would trigger an error "Attempting to run JS driven animation on animated". That is because for natively animated nodes we don't allow for `setNativeProps` to be executed and method `_flush` is now responsible for triggering that call. To fix the issue we add extra flag to `_updateValue` method that indicates if we should be "flushing" updated values using `setNativeProps` and we pass an appropriate value depending on the status of the node (native/non-native). Note that in animation callback we always pass `true` - that is because natively driven animations will never call into that callback.

**Test Plan**
Run JS tests: `npm test Libraries/Animated/src/__tests__/AnimatedNative-test.js`
Closes facebook#7138

Differential Revision: D3257696

fb-gh-sync-id: 13ec26bc36b56b3208f4279a95532bbe60551087
fbshipit-source-id: 13ec26bc36b56b3208f4279a95532bbe60551087
  • Loading branch information
kmagiera authored and Facebook Github Bot 7 committed May 4, 2016
1 parent d4cc5b5 commit 6c80f88
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 6 deletions.
12 changes: 8 additions & 4 deletions Libraries/Animated/src/AnimatedImplementation.js
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ class AnimatedValue extends AnimatedWithChildren {
this._animation.stop();
this._animation = null;
}
this._updateValue(value);
this._updateValue(value, !this.__isNative /* don't perform a flush for natively driven values */);
if (this.__isNative) {
NativeAnimatedAPI.setAnimatedNodeValue(this.__getNativeTag(), value);
}
Expand Down Expand Up @@ -730,7 +730,9 @@ class AnimatedValue extends AnimatedWithChildren {
animation.start(
this._value,
(value) => {
this._updateValue(value);
// Natively driven animations will never call into that callback, therefore we can always pass `flush = true`
// to allow the updated value to propagate to native with `setNativeProps`
this._updateValue(value, true /* flush */);
},
(result) => {
this._animation = null;
Expand Down Expand Up @@ -760,9 +762,11 @@ class AnimatedValue extends AnimatedWithChildren {
this._tracking = tracking;
}

_updateValue(value: number): void {
_updateValue(value: number, flush: bool): void {
this._value = value;
_flush(this);
if (flush) {
_flush(this);
}
for (var key in this._listeners) {
this._listeners[key]({value: this.__getValue()});
}
Expand Down
21 changes: 19 additions & 2 deletions Libraries/Animated/src/__tests__/AnimatedNative-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,27 @@ describe('Animated', () => {
var anim = new Animated.Value(0);
Animated.timing(anim, {toValue: 10, duration: 1000, useNativeDriver: true}).start();

anim.setValue(5);
var c = new Animated.View();
c.props = {
style: {
opacity: anim,
},
};
c.componentWillMount();

// We expect `setValue` not to propagate down to `setNativeProps`, otherwise it may try to access `setNativeProps`
// via component refs table that we override here.
c.refs = {
node: {
setNativeProps: jest.genMockFunction(),
},
};

anim.setValue(0.5);

var nativeAnimatedModule = require('NativeModules').NativeAnimatedModule;
expect(nativeAnimatedModule.setAnimatedNodeValue).toBeCalledWith(jasmine.any(Number), 5);
expect(nativeAnimatedModule.setAnimatedNodeValue).toBeCalledWith(jasmine.any(Number), 0.5);
expect(c.refs.node.setNativeProps.mock.calls.length).toBe(0);
});

it('doesn\'t call into native API if useNativeDriver is set to false', () => {
Expand Down

0 comments on commit 6c80f88

Please sign in to comment.