-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Make Vibration.vibrate compatible with TurboModules #27951
Conversation
This is a great PR. Thanks for fixing 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.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Ran into some issues landing this, which only surfaced internally due to TurboModules not being fully supported in open source yet.
Do you think you can fix that up in your PR? |
@hramos Sure, np. I'll take a look some time this week. |
@brunobar79 bump, if you want this to make it into v0.62.0-rc.3, which we'll cut over the next few days. |
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.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Imported, but it ran into some issues. I'll look at this again sometime this week. |
@hramos The same issue exists on 0.62.0. Is there any schedule when this pr could be merged? :) |
Note: This error only exists on debug builds, it seems to work fine in release mode, for some reasons.. This PR fixes the issue. |
This is still on my todo list, and the past month kind of threw things off. I'll try to take another look soon. |
We're actively working on landing this change. |
This is landing now. Thanks to @TheSavior for helping push this through. |
This pull request was successfully merged by @brunobar79 in 3904228. When will my fix make it into a release? | Upcoming Releases |
Summary: This PR fixes a compatibility issue with the Vibration module and TurboModules. The TurboModules spec doesn't allow nullable arguments of type Number, causing the following problem: ![IMG_3758](https://user-images.githubusercontent.com/1247834/73803879-10be6f80-4790-11ea-92d4-a008f0007681.PNG) [iOS] [Fixed] - Make Vibration library compatible with TurboModules. Pull Request resolved: #27951 Test Plan: Just submitted a PR to my own app to fix the issue [here](rainbow-me/rainbow#340) The problem should be reproducible on RNTester due to this line: https://github.com/facebook/react-native/blob/91f139b94118fe8db29728ea8ad855fc4a13f743/RNTester/js/examples/Vibration/VibrationExample.js#L66 and should be working on this branch. Reviewed By: TheSavior Differential Revision: D19761064 Pulled By: hramos fbshipit-source-id: 84f6b62a2734cc09d450e906b5866d4e9ce61124
Summary
This PR fixes a compatibility issue with the Vibration module and TurboModules.
The TurboModules spec doesn't allow nullable arguments of type Number, causing the following problem:
Changelog
[iOS] [Fixed] - Make Vibration library compatible with TurboModules.
Test Plan
Just submitted a PR to my own app to fix the issue here
The problem should be reproducible on RNTester due to this line:
react-native/RNTester/js/examples/Vibration/VibrationExample.js
Line 66 in 91f139b