-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add listener for non-value animated node #22883
Add listener for non-value animated node #22883
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
@@ -259,6 +176,10 @@ class AnimatedValue extends AnimatedWithChildren { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type annotation for value
.
@@ -321,9 +242,7 @@ class AnimatedValue extends AnimatedWithChildren { | |||
if (flush) { | |||
_flush(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call this._callListeners
because property _callListeners
is missing in AnimatedValue
[1].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's in superclass
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
@@ -321,9 +242,7 @@ class AnimatedValue extends AnimatedWithChildren { | |||
if (flush) { | |||
_flush(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call this._callListeners
because property _callListeners
is missing in AnimatedValue
[1].
iOS tests appear to fail in not-related places |
can you rebase against master to see if the latest CI fixes there make this PR go green? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
@@ -321,9 +242,7 @@ class AnimatedValue extends AnimatedWithChildren { | |||
if (flush) { | |||
_flush(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call this._callListeners
because property _callListeners
is missing in AnimatedValue
[1].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failures are unrelated, probably another merge from master would work now, but I'd like to see if any issues are found in FB's CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
CI is failing because of a Flow error, so this definitely doesn't seem to be ready yet. At FB, it's failing because we access |
Merge changes from master
* | ||
* See http://facebook.github.io/react-native/docs/animatedvalue.html#addlistener | ||
*/ | ||
addListener(callback: any): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not perfect to use any
, but we need to override this type in AnimatedValueXY
with another type which is related to another logic. 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osdnk maybe we can use callback: (value: any) => mixed
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callback: (value: any) => void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osdnk using mixed
or any
is better in this case. void
will prevent async functions to be used for the listener: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVUCuA7AxgFwEs5swBDAEwoDU5CKAZQgZ3wFNs2AnAClzJgwARmVwBrAFxgeANwGY2UstgCeASjABeAHxgZdChoDeAX3RY8REuSoBZQgA82jFu069+gkeKmz5i8lUNHTAAW0dnYzNUShoDJlYObh4yZhU8aWDdI1QwMGBgMAo4MGY4ULZ8AAtCbABzVBM1AG50WPsnF0T3FLSMniywHLyCopKyiurahqbmoA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, well, I didn't want to do it previously, cause it changes existing code
cc @mattharget let me know when this is ready for shipping |
@cpojer @matthargett forgot second "t" 😃 |
Still waiting for a review on this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests and fixing the flow issues!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give this a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @osdnk in 68a5cee. When will my fix make it into a release? | Upcoming Releases |
Summary: Changelog: ---------- [Changed][General] Move callback-related logic to `AnimatedNode` class in order to make it possible to add the listener for other animated nodes than `AnimatedValue`. I observed that native code appears to be fully prepared for listening not only to animated value but animated nodes generally. Therefore I managed to modify js code for exposing `addListener` method from `AnimatedNode` class instead of `AnimatedValue`. It called for some minor changes, which are not breaking. If you're fine with these changes, I could add proper docs if needed. Pull Request resolved: facebook#22883 Differential Revision: D14041747 Pulled By: cpojer fbshipit-source-id: 94c68024ceaa259d9bb145bf4b3107af0b15db88
Summary: Changelog: ---------- [Changed][General] Move callback-related logic to `AnimatedNode` class in order to make it possible to add the listener for other animated nodes than `AnimatedValue`. I observed that native code appears to be fully prepared for listening not only to animated value but animated nodes generally. Therefore I managed to modify js code for exposing `addListener` method from `AnimatedNode` class instead of `AnimatedValue`. It called for some minor changes, which are not breaking. If you're fine with these changes, I could add proper docs if needed. Pull Request resolved: #22883 Differential Revision: D14041747 Pulled By: cpojer fbshipit-source-id: 94c68024ceaa259d9bb145bf4b3107af0b15db88
Changelog:
[Changed][General] Move callback-related logic to
AnimatedNode
class in order to make it possible to add the listener for other animated nodes thanAnimatedValue
.I observed that native code appears to be fully prepared for listening not only to animated value but animated nodes generally. Therefore I managed to modify js code for exposing
addListener
method fromAnimatedNode
class instead ofAnimatedValue
. It called for some minor changes, which are not breaking.If you're fine with these changes, I could add proper docs if needed.
Test Plan:
The most simple example to verify whether it works is to use code like this.
You might switch
useNativeDriver
in order to verify my solution with and without native driver support.