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.

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

adds type checking to Animated toValue and iterations key values to m… #23812

Conversation

andrew-schenk
Copy link

…ake sure Android does not crash from bad params when using useNativeDriver

Summary

Android apps crash when using Animated useNativeDriver: true and the toValue is not a number. See issue here with test case. Issue

Changelog

[Android] [fixed] - Fix crash when using Animated with useNativeDriver and a non Number toValue

Test Plan

Run Snack code with patched React Native core. Malformed parameters will no longer crash Android app.

…ake sure Android does not crash from bad params when using useNativeDriver
@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 Mar 7, 2019
@matthargett
Copy link
Contributor

Should we show a warning in dev mode when this is the case? It a user has a bug in their JS, they may get confused when zeroes end up getting used silently.

@andrew-schenk
Copy link
Author

My motivation for fixing this is that the JS bug is in one of possibly 100 different dependency libraries. Showing a warning might help focus attention on which library is the culprit or where in the internal project code the developer went awry. On the other hand, it might be another yellow box warning that gets silenced because the 3rd party lib is unsupported and the developer is tired of getting nagged about it.

Personally, I would probably want to see a warning and have the option of silencing it or fixing the library.

@AndrewSchenk
Copy link

AndrewSchenk commented Mar 10, 2019

The JavaScript animated version does not show a yellow box warning if you supply an invalid type. i.e.

toValue: null,
useNativeDriver: false

So to maintain parity between JS and native it would be best to not show a yellow box.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 13, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andrewschenk-linx in 25d3976.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added Merged This PR has been merged. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Mar 13, 2019
@andrew-schenk andrew-schenk deleted the Animated-useNativeDriver-fix-to-Value branch March 20, 2019 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants