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

Make Vibration.vibrate compatible with TurboModules #27951

Closed

Conversation

brunobar79
Copy link
Contributor

@brunobar79 brunobar79 commented Feb 5, 2020

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

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:

onPress={() => Vibration.vibrate()}>
and should be working on this branch.

@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 Feb 5, 2020
@brunobar79 brunobar79 changed the title Fix vibration turbomodules Make Vibration.vibrate compatible with TurboModules. Feb 5, 2020
@brunobar79 brunobar79 changed the title Make Vibration.vibrate compatible with TurboModules. Make Vibration.vibrate compatible with TurboModules Feb 5, 2020
@hramos
Copy link
Contributor

hramos commented Feb 6, 2020

This is a great PR. Thanks for fixing this!

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.

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Feb 6, 2020
@hramos
Copy link
Contributor

hramos commented Feb 7, 2020

Ran into some issues landing this, which only surfaced internally due to TurboModules not being fully supported in open source yet.

  1. Had to regenerate the native Java spec for the Vibration TurboModule, since the JS spec changed. There's currently no way to run this codegen in open source, but we're working on it.
  2. In turn, this surfaced the following error, related to the generated method signatures having changed on Android:
stderr: ReactAndroid/src/main/java/com/facebook/react/modules/vibration/VibrationModule.java:20: error: com.facebook.react.modules.vibration.VibrationModule is not abstract and does not override abstract method vibrate(double) in com.facebook.fbreact.specs.NativeVibrationSpec
public class VibrationModule extends NativeVibrationSpec {
       ^
ReactAndroid/src/main/java/com/facebook/react/modules/vibration/VibrationModule.java:33: error: method does not override or implement a method from a supertype
  @Override
  ^

Do you think you can fix that up in your PR?

@brunobar79
Copy link
Contributor Author

@hramos Sure, np. I'll take a look some time this week.

@alloy
Copy link
Contributor

alloy commented Feb 20, 2020

@brunobar79 bump, if you want this to make it into v0.62.0-rc.3, which we'll cut over the next few days.

@brunobar79
Copy link
Contributor Author

brunobar79 commented Feb 20, 2020

@hramos Can you give it a try now? If you regenerate the Java Spec it should match.
cc: @alloy

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.

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hramos
Copy link
Contributor

hramos commented Feb 25, 2020

Imported, but it ran into some issues. I'll look at this again sometime this week.

@Frans-L
Copy link

Frans-L commented Mar 30, 2020

@hramos The same issue exists on 0.62.0. Is there any schedule when this pr could be merged? :)

@maxencehenneron
Copy link

Note: This error only exists on debug builds, it seems to work fine in release mode, for some reasons.. This PR fixes the issue.

@hramos
Copy link
Contributor

hramos commented Apr 6, 2020

This is still on my todo list, and the past month kind of threw things off. I'll try to take another look soon.

@hramos
Copy link
Contributor

hramos commented Apr 7, 2020

We're actively working on landing this change.

@hramos
Copy link
Contributor

hramos commented Apr 8, 2020

This is landing now. Thanks to @TheSavior for helping push this through.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @brunobar79 in 3904228.

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 Needs: Imported Diff Waiting on Meta labels Apr 8, 2020
alloy pushed a commit that referenced this pull request Apr 8, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 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. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants