AnimatedWithChildren support for addListener #12620

scarlac · 2017-02-28T15:14:02Z

Motivation

This PR is born out of trying to move to useNativeDriver. It addresses the issue that manipulations with Animated.add, .multiply, etc. would not return an Animated.Value which meant it could not be used for attaching listeners. Animated.event with native driver also doesn't support listeners either.

This PR mostly moved code from Animated.Value into AnimatedWithChildren. There are some edge cases which may need to be addressed but I feel like this PR can get some discussion started on where to take the changes. Not being able to add listeners to animation child nodes currently limits the usability of native animations.

Test plan (required)

I have a local project that has been transitioned to useNativeDriver. This is how I am currently testing my code. I think we should discuss if this is the right way to go forward before writing extensive tests.

This would address the issue that manipulations with Animated.add, .multiply, etc. would not return an Animated.Value which meant it could not be used for attaching listeners.

scarlac · 2017-02-28T15:19:36Z

Ping @janicduplessis . This is the PR I mentioned on Twitter. I don't know what the plans are for the native driver with listeners so forgive me if this is a naive solution. I expect we may need to talk it over, depending on what your plans are.

eslint-bot · 2017-02-28T16:52:46Z

Libraries/Animated/src/AnimatedImplementation.js

-    NativeAnimatedAPI.stopListeningToAnimatedNodeValue(this.__getNativeTag());
-  }
-
  /**


parameter data Missing annotation

eslint-bot · 2017-02-28T16:52:46Z

Libraries/Animated/src/AnimatedImplementation.js

+
+  /* Typically used internally */
+  triggerListeners(eventPropagation?: bool = true): void {
+    for (var key in this._listeners) {


keyword-spacing: Expected space(s) after "if".

eslint-bot · 2017-02-28T16:52:46Z

Libraries/Animated/src/AnimatedImplementation.js

+  triggerListeners(eventPropagation?: bool = true): void {
+    for (var key in this._listeners) {
+      this._listeners[key]({value: this.__getValue()});
+    }


keyword-spacing: Expected space(s) after "if".

janicduplessis · 2017-02-28T19:44:56Z

Nice! Seems like a great solution, I like how you implemented the JS part with event propagation since every node is not a value node like in the native implementation. One thing that could be improved is make sure the value actually changed before triggering the change event and stop the propagation too. I'm pretty sure native does that too so it would make sure the behavior is exactly the same.

eslint-bot · 2017-02-28T23:27:58Z

Libraries/Animated/src/AnimatedImplementation.js

+
+  /* Typically used internally */
+  triggerListeners(eventPropagation?: bool = true): void {
+    for (var key in this._listeners) {


keyword-spacing: Expected space(s) after "if".

eslint-bot · 2017-02-28T23:27:58Z

Libraries/Animated/src/AnimatedImplementation.js

-    NativeAnimatedAPI.stopListeningToAnimatedNodeValue(this.__getNativeTag());
-  }
-
  /**


parameter data Missing annotation

eslint-bot · 2017-02-28T23:27:59Z

Libraries/Animated/src/AnimatedImplementation.js

+  triggerListeners(eventPropagation?: bool = true): void {
+    for (var key in this._listeners) {
+      this._listeners[key]({value: this.__getValue()});
+    }


keyword-spacing: Expected space(s) after "if".

janicduplessis · 2017-03-01T19:26:24Z

LGTM, could you just fix lint and flow errors.

scarlac · 2017-03-01T21:55:42Z

Ofc! I fixed the lint issues but I can't reproduce the flow error "function type Callable signature not found in object type" locally. I'll wait for the eslint-bot to re-run and then see what comes out.

janicduplessis · 2017-03-01T22:01:11Z

Have you run flow with npm run flow? This will make sure to use the local flow-bin which uses flow 0.40.

scarlac · 2017-03-01T22:06:18Z

Forgot to add --show-all-errors :) Re-checking!

Flow (correctly) detected that listeners were not compatible in AnimatedXYValue, so this change should ensure that both work. It should also ensure that all listeners are correctly tracked and removed after use.

janicduplessis · 2017-03-06T01:28:58Z

Tested this in an app and it works really well! Did you manage to fix the flow errors?

scarlac · 2017-03-06T08:19:43Z

Great! Yes, I did. The patch following your comment were to fix the flow issues :)

janicduplessis · 2017-03-06T08:41:08Z

Looks like there is still flow errors in other files caused by this change. https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/NavigationExperimental/NavigationCardStackPanResponder.js#L232 is the issue but it might be a bug in flow because the property does exist since it is defined in the parent class. Maybe you could just add a $FlowFixMe comment since it seems like a flow bug and this code is hacky and uses Animated internals.

janicduplessis · 2017-03-06T08:43:48Z

@ericvicenti Could you have a look at these flow errors to see if I'm missing something or this might be a bug in flow?

scarlac · 2017-03-06T09:07:41Z

If I type hint it to _addNativeListener(animatedValue:Animated.Value) Flow recognizes the correct type but still claims _listeners is undefined. It clearly knows the type, but fails to recognize that _listeners is initialized in the parent class.

The error is:

Libraries/CustomComponents/NavigationExperimental/NavigationCardStackPanResponder.js:232
232:     if (Object.keys(animatedValue._listeners).length === 0) {
                                       ^^^^^^^^^^ property `_listeners`. Property not found in
232:     if (Object.keys(animatedValue._listeners).length === 0) {
                         ^^^^^^^^^^^^^ AnimatedValue

My best guess is that Flow doesn't recognize when setting instance properties through super() ?

ericvicenti · 2017-03-06T09:08:05Z

It looks like the issue is that CardStackPanResponder attempts to access a private _listeners property on the animated value.

janicduplessis · 2017-03-06T09:11:13Z

@ericvicenti What is weird is that it used to work before moving the property to the parent class.

scarlac · 2017-03-06T09:43:22Z

A workaround would be to re-define the type in AnimatedValue (ie. _listeners: AnimatedWithChildren._listeners;) without initializing it with a value. I haven't tested it thoroughly but it should work and it does silence Flow. Let me know if you want me to go further with this workaround.

janicduplessis · 2017-03-06T13:51:02Z

I'd rather just fix it in PanResponder. You could annotate the function with Object instead of Animated.Value and it should work.

Relating to #12620 (comment) 83

janicduplessis · 2017-03-06T19:27:45Z

Libraries/Animated/src/AnimatedImplementation.js

 }

-type ValueListenerCallback = (state: {value: number}) => void;
+type AnimatedListenerCallback = (state: Object) => void;


Any reason for this change? I'd rather keep the more precise flow type.

Since the callbacks are now added by the parent class (AnimatedWithChildren), the arguments are now up to the extending classes, which means AnimatedValueXY takes {x: number, y:number} while AnimatedValue takes {value: number}.
I'd like to keep the types but I'm not familiar enough with Flow to know how that would work. Should I investigate further?

I see, does keeping (state: {value: number}) => void in AnimatedWithChildren and using a different type for AnimatedValueXY works? If not I don't really have a better idea :(

Actually, after the flow fixes your suggestion may work (listeners for AnimatedValue and ~XY were decoupled). I'll brb with a fix and change the name back as well, to keep the changes minimal.

janicduplessis · 2017-03-06T19:28:59Z

Libraries/Animated/src/AnimatedImplementation.js

-    for (var key in this._listeners) {
-      this._listeners[key]({value: this.__getValue()});
-    }
+    this.triggerListeners();


Let's prefix this method with an underscore since it's not really public API.

scarlac · 2017-03-06T23:12:34Z

I've made it a union type and renamed it back to ValueListenerCallback. This is the best solution I could find, since child classes cannot overload the parent class method and change their type - or at least Flow doesn't allow for it. The net effect is simply that the type checking is stricter, but it won't catch if someone supplies a ({x:number, y:number}) => void callback to a regular AnimatedValue that's going to pass {value:number} instead.

scarlac · 2017-03-14T13:53:51Z

@janicduplessis Let me know if you need any more fixes to get it merged :)

Relating to facebook#12620 (comment) 83

janicduplessis · 2017-03-24T00:10:57Z

I tested this with flow and the issue is that the argument of the listener callback is a union type now so you will have to check the type before using it. I guess I can see 2 solutions:

Move most of the logic to some internal method in AnimatedWithChildren and have all subclasses implement the addListener / removeListener with their own signature that just calls the internal method.
Go back with the object type instead of the union type but we lose out on good flow typing.

scarlac · 2017-04-21T10:02:51Z

If we move the implementation to all subclasses, then we fragment everything relating to listeners within AnimatedWithChildren, effectively nullifying the changes and I'd have to rewrite everything, duplicating code along the way. The listeners would then have to be in the subclasses, etc. The issue here is how method signatures are handled in JavaScript and Flow.
My suggestion is to keep using the object union type in the patch and get this merged. What's your take now that some time as passed, @janicduplessis ?

facebook-github-bot · 2017-08-06T17:03:53Z

@scarlac I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

juangl · 2017-10-03T16:02:29Z

Hey, @janicduplessis. Is there any progress here?

janicduplessis · 2017-10-03T20:12:38Z

@juangl Not at the moment, I'll try to find some time soon to revisit this.

sgavade · 2017-11-07T23:50:49Z

Hi @janicduplessis , Any idea when this PR will be merged?

stale · 2018-01-07T00:14:58Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

slorber · 2018-08-02T07:48:13Z

this useful PR has been closed :(

Yet I think we really need this to land in RN to make some animations easier.

What should we do to make progress on this? make a new PR or ask to reopen this one?

scarlac · 2018-08-02T08:00:18Z

Someone would need to rebase this off of master and test it thoroughly. Flow has also been updated quite a bit since then so maybe there are options to make it work better.

…

On Thu, 2 Aug 2018 at 00:49, Sébastien Lorber ***@***.***> wrote: this useful PR has been closed :( Yet I think we really need this to land in RN to make some animations easier. What should we do to make progress on this? make a new PR or ask to reopen this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12620 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2piQbeNQuIlowCdvqb4q6fh3vL2nCAks5uMq8hgaJpZM4MOhEk> .

cglacet · 2022-11-24T15:30:21Z

Should it work in recent versions? (I'm using 0.67.2 and have this issue React native add listener on Animated.add value)

scarlac · 2022-11-24T15:43:57Z

@cglacet I don't know but I recommend reanimated 2

cglacet · 2022-11-25T10:23:29Z

@scarlac Yeah maybe, but switching to reanimated is a very heavy change that involves a lot of risks (app crashes). If I could avoid this I would really love to.

AnimatedWithChildren support for addListener

6d3b05d

This would address the issue that manipulations with Animated.add, .multiply, etc. would not return an Animated.Value which meant it could not be used for attaching listeners.

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Feb 28, 2017

eslint-bot reviewed Feb 28, 2017

View reviewed changes

Lint fixes

2cc5d3b

scarlac and others added 3 commits March 2, 2017 11:53

Fixed type inconsistencies with AnimatedXYValue

f518dc9

Flow (correctly) detected that listeners were not compatible in AnimatedXYValue, so this change should ensure that both work. It should also ensure that all listeners are correctly tracked and removed after use.

Merge branch 'master' into patch-4

7f3c21d

Merge branch 'master' into patch-4

c7b0128

Workaround Flow bug: _listeners does exist

514496e

Relating to #12620 (comment) 83

janicduplessis reviewed Mar 6, 2017

View reviewed changes

scarlac added 2 commits March 6, 2017 20:35

triggerListeners is not a public method

bebdf03

Union type for slightly stricter type checking of callbacks

a0b8473

Merge branch 'master' into patch-4

f28b941

janicduplessis pushed a commit to janicduplessis/react-native that referenced this pull request Mar 18, 2017

Workaround Flow bug: _listeners does exist

fa78ddd

Relating to facebook#12620 (comment) 83

janicduplessis pushed a commit to janicduplessis/react-native that referenced this pull request Mar 23, 2017

Workaround Flow bug: _listeners does exist

13ddeff

Relating to facebook#12620 (comment) 83

janicduplessis pushed a commit to janicduplessis/react-native that referenced this pull request Mar 23, 2017

Workaround Flow bug: _listeners does exist

8106930

Relating to facebook#12620 (comment) 83

shergin added the JavaScript label May 31, 2017

janicduplessis mentioned this pull request Jun 10, 2017

Use native animations for DefaultTabBar ptomasroos/react-native-scrollable-tab-view#661

Merged

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 7, 2018

stale bot closed this Jan 14, 2018

bingoo2008 mentioned this pull request May 30, 2018

将该组件嵌套在ScrollView里在安卓上无法显示 ptomasroos/react-native-scrollable-tab-view#881

Closed

AnimatedWithChildren support for addListener #12620

AnimatedWithChildren support for addListener #12620

Uh oh!

Conversation

scarlac commented Feb 28, 2017

Uh oh!

scarlac commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Mar 1, 2017

Uh oh!

scarlac commented Mar 1, 2017

Uh oh!

janicduplessis commented Mar 1, 2017

Uh oh!

scarlac commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Mar 6, 2017

Uh oh!

scarlac commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Mar 6, 2017

Uh oh!

janicduplessis commented Mar 6, 2017

Uh oh!

scarlac commented Mar 6, 2017

Uh oh!

ericvicenti commented Mar 6, 2017

Uh oh!

janicduplessis commented Mar 6, 2017

Uh oh!

scarlac commented Mar 6, 2017

Uh oh!

janicduplessis commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scarlac commented Mar 6, 2017

Uh oh!

scarlac commented Mar 14, 2017

Uh oh!

janicduplessis commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scarlac commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Aug 6, 2017

Uh oh!

juangl commented Oct 3, 2017

Uh oh!

janicduplessis commented Oct 3, 2017

Uh oh!

sgavade commented Nov 7, 2017

Uh oh!

stale bot commented Jan 7, 2018

Uh oh!

scarlac commented Mar 1, 2017 •

edited

Loading

scarlac commented Mar 6, 2017 •

edited

Loading

janicduplessis commented Mar 24, 2017 •

edited

Loading

scarlac commented Apr 21, 2017 •

edited

Loading

cglacet commented Nov 25, 2022 •

edited

Loading