Uh oh!
There was an error while loading. Please reload this page.
[NativeAnimated][iOS] Add support for value listener - #9194
[NativeAnimated][iOS] Add support for value listener#9194janicduplessis wants to merge 3 commits into
Conversation
ghost
commented
Aug 3, 2016
ryangomba
commented
Aug 3, 2016
@janicduplessis oh snap, thanks for doing this! |
ryangomba
commented
Aug 3, 2016
Circle is unhappy, but otherwise looks good to me. Will wait until #8844 is landed. |
facebook-github-bot
commented
Aug 3, 2016
@janicduplessis updated the pull request. |
janicduplessis
commented
Aug 3, 2016
Ok tests should pass now, NativeEventEmitter triggered an invariant because native modules are not defined during tests so I just mocked it. |
6bb8973 to
90ede19Comparefacebook-github-bot
commented
Aug 3, 2016
@janicduplessis updated the pull request. |
ide
commented
Aug 4, 2016
@janicduplessis can you rebase? #8844 has been merged too. |
90ede19 to
5e19eeaCompareghost
commented
Aug 5, 2016
@janicduplessis updated the pull request. |
janicduplessis
commented
Aug 5, 2016
@ide Done! |
ide
commented
Aug 5, 2016
@facebook-github-bot shipit |
brentvatne
commented
Aug 7, 2016
@facebook-github-bot shipit |
ghost
commented
Aug 7, 2016
Thanks for importing.If you are an FB employee go to Phabricator to review internal test results. |
ghost
commented
Aug 7, 2016
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
ghost
commented
Aug 8, 2016
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
janicduplessis
commented
Aug 8, 2016
@javache Could you check why the import failed? There is no conflict and OSS tests all passed. |
There was a problem hiding this comment.
@janicduplessis just a guess, maybe FB is relying on this. We could stub it out as return true and try to merge.
There was a problem hiding this comment.
I don't think so, I added it in the Android PR a few days ago.
brentvatne
commented
Aug 10, 2016
@foghina maybe :) |
foghina
commented
Aug 11, 2016
Had a look but this is too iOS-y for me. A build seems to be failing with (I think) this error: Not sure if this is useful in any way. Leaving this up to @javache to figure out. |
janicduplessis
commented
Aug 11, 2016
I think I figured out why, some internal apps probably don't include the native |
ide
commented
Aug 11, 2016
Thanks @foghina, that stack trace is helpful. "evalmachine" is part of Node's vm module, perhaps this is failing in a Jest test? |
e596fb6 to
98d0553Compareghost
commented
Aug 11, 2016
@janicduplessis updated the pull request. |
facebook-github-bot
commented
Aug 11, 2016
@janicduplessis updated the pull request. |
janicduplessis
commented
Aug 11, 2016
@ide Looks like it, I had to mock NativeEventEmitter in a test to make it pass but now I changed the NativeEventEmitter creation to be lazy so it won't need to be mocked unless the test uses an animated value listener for a native driven node. |
ide
commented
Aug 11, 2016
@facebook-github-bot shipit |
ghost
commented
Aug 11, 2016
Thanks for importing.If you are an FB employee go to Phabricator to review internal test results. |
Summary: Adds support for `Animated.Value#addListener` for native driven animated values. Same as #8844 but for iOS. This depends on some JS code in #8844 so only review the 2nd commit and let's wait for #8844 to land first. **Test plan** Tested using the UIExplorer example. Closesreact/react-native#9194 Differential Revision: D3681749 fbshipit-source-id: 521a61e2221c1ad1f6f40c75dd2dc957361d0271
Adds support for
Animated.Value#addListenerfor native driven animated values. Same as #8844 but for iOS. This depends on some JS code in #8844 so only review the 2nd commit and let's wait for #8844 to land first.Test plan
Tested using the UIExplorer example.