Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[NativeAnimated][iOS] Add support for value listener #9194

Closed
wants to merge 3 commits into from
Closed

[NativeAnimated][iOS] Add support for value listener #9194

wants to merge 3 commits into from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Aug 3, 2016 •
edited
Loading

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.

@ghost
Copy link

ghost commented Aug 3, 2016

By analyzing the blame information on this pull request, we identified @buba447 and @kmagiera to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 3, 2016
@ryangomba
Copy link
Contributor

@janicduplessis oh snap, thanks for doing this!

@ryangomba
Copy link
Contributor

Circle is unhappy, but otherwise looks good to me. Will wait until #8844 is landed.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@janicduplessis
Copy link
Contributor Author

Ok tests should pass now, NativeEventEmitter triggered an invariant because native modules are not defined during tests so I just mocked it.

@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@ide
Copy link
Contributor

ide commented Aug 4, 2016

@janicduplessis can you rebase? #8844 has been merged too.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 4, 2016
@ghost
Copy link

ghost commented Aug 5, 2016

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2016
@janicduplessis
Copy link
Contributor Author

@ide Done!

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2016
@ide
Copy link
Contributor

ide commented Aug 5, 2016

@facebook-github-bot shipit

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2016
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2016
@brentvatne
Copy link
Collaborator

@facebook-github-bot shipit

@ghost ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 7, 2016
@ghost
Copy link

ghost commented Aug 7, 2016

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 7, 2016
@ghost
Copy link

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 ghost added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 7, 2016
@ghost
Copy link

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.

@ghost ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 8, 2016
@janicduplessis
Copy link
Contributor Author

@javache Could you check why the import failed? There is no conflict and OSS tests all passed.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 8, 2016
ide
ide reviewed Aug 9, 2016
View reviewed changes
Libraries/Animated/src/NativeAnimatedHelper.js
@@ -166,5 +164,5 @@ module.exports = {
generateNewNodeTag,
generateNewAnimationId,
assertNativeAnimatedModule,
supportsNativeListener,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janicduplessis just a guess, maybe FB is relying on this. We could stub it out as return true and try to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, I added it in the Android PR a few days ago.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 9, 2016
@brentvatne
Copy link
Collaborator

@foghina maybe :)

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 10, 2016
@foghina
Copy link
Contributor

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:

Invariant Violation: Native module cannot be null.
    at invariant (evalmachine.<anonymous>:2406:7)
    at new NativeEventEmitter (evalmachine.<anonymous>:4459:51)
    at evalmachine.<anonymous>:71282:24
    at loadModuleImplementation (evalmachine.<anonymous>:127:1)
    at guardedLoadModule (evalmachine.<anonymous>:70:8)
    at _require (evalmachine.<anonymous>:54:1)
    at evalmachine.<anonymous>:69201:23
    at loadModuleImplementation (evalmachine.<anonymous>:127:1)
    at guardedLoadModule (evalmachine.<anonymous>:70:8)
    at _require (evalmachine.<anonymous>:54:1)

Not sure if this is useful in any way. Leaving this up to @javache to figure out.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 11, 2016
@janicduplessis
Copy link
Contributor Author

I think I figured out why, some internal apps probably don't include the native AnimatedModule and this creates a NativeEventEmitter even if we don't use it. I'll make the nativeEventEmitter creation lazy that way it won't get created for apps that don't use it.

@ide
Copy link
Contributor

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?

@ghost
Copy link

ghost commented Aug 11, 2016

@janicduplessis updated the pull request.

@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 11, 2016
@janicduplessis
Copy link
Contributor Author

@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.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 11, 2016
@ide
Copy link
Contributor

ide commented Aug 11, 2016

@facebook-github-bot shipit

@ghost ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 11, 2016
@ghost
Copy link

ghost commented Aug 11, 2016

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 11, 2016
@ghost ghost closed this in 0e204e1 Aug 12, 2016
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
@janicduplessis
Add support for value listener
871b701 
Summary:
Adds support for `Animated.Value#addListener` for native driven animated values. Same as facebook#8844 but for iOS. This depends on some JS code in facebook#8844 so only review the 2nd commit and let's wait for facebook#8844 to land first.

**Test plan**
Tested using the UIExplorer example.
Closes facebook#9194

Differential Revision: D3681749

fbshipit-source-id: 521a61e2221c1ad1f6f40c75dd2dc957361d0271
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Aug 25, 2016
@janicduplessis
Add support for value listener
11e622c 
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.
Closes facebook/react-native#9194

Differential Revision: D3681749

fbshipit-source-id: 521a61e2221c1ad1f6f40c75dd2dc957361d0271
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@javache javache

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@janicduplessis @ryangomba @facebook-github-bot @ide @brentvatne @foghina @javache