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

Changed proguard rule for Hermes is not specified in the upgrade helper for 0.62. #19

Closed
yghokim opened this issue Mar 28, 2020 · 10 comments · Fixed by react-native-community/upgrade-helper#224

Comments

@yghokim
Copy link

yghokim commented Mar 28, 2020

Feature Request

The new proguard rule for using Hermes:

-keep class com.facebook.jni.** { *; }

is not mentioned in the upgrade helper for 0.62.

Why it is needed

Omitting this update makes the Android release app with Hermes and Proguard crashes on startup:

FATAL EXCEPTION: create_react_context
java.lang.UnsatisfiedLinkError: couldn't find DSO to load: libhermes.so caused by: com.facebook.jni.NativeRunnable
@pvinis
Copy link
Member

pvinis commented Mar 29, 2020

Where is that change exactly? I couldn't find this in a new project.

@pvinis pvinis transferred this issue from react-native-community/upgrade-helper Mar 29, 2020
@pvinis pvinis added the 0.62.0 label Mar 29, 2020
@yghokim
Copy link
Author

yghokim commented Mar 29, 2020

In the React Native documentation of 0.61,
The Hermes setup page (https://reactnative.dev/docs/0.61/hermes) specified this Proguard rule:

-keep class com.facebook.hermes.unicode.** { *; }

In the documentation of 0.62 (https://reactnative.dev/docs/hermes), the Proguard rule is updated:

-keep class com.facebook.hermes.unicode.** { *; }
-keep class com.facebook.jni.** { *; }

I think this change was made because 0.62 uses Hermes 0.4.0.

However, I spent hours struggling with a crash bug because this Proguard.rule change was not specified in the update helper.

If it is specified somewhere, it should be more salient for developers who are using Hermes.

@lucasbento
Copy link
Member

I wonder how we can add this to the upgrade-helper, I was thinking that we can add an inline comment here:
image

(reason is that we can only add inline comments on diffs)

And maybe add this issue at the top with a title like "Upgrade proguard rule for Hermes".

Thoughts on this @muclipse @pvinis?

@pvinis
Copy link
Member

pvinis commented Mar 30, 2020

Right. I checked the proguard-rules.pro file, and you can also see here, there is nothing in there for hermes like the docs say. The way the upgrade-helper and purge work is that they diff the template between versions, so if there was no change, nothing will be shown. The change is in the documentation.

I feel this change might be more suitable in the releases changelog.

For now, probably the best we can do is what @lucasbento said, and we can add a comment or a note on the 0.62 upgrade helper.

@lucasbento
Copy link
Member

Right. I checked the proguard-rules.pro file, and you can also see here , there is nothing in there for hermes like the docs say.

Yeah, this is probably because the template does not use Hermes by default.

I feel this change might be more suitable in the releases changelog .

Agreed, cc @alloy.

@yghokim
Copy link
Author

yghokim commented Mar 30, 2020

I feel this change might be more suitable in the releases changelog .

I agree, @alloy.

Also,

we can add a comment or a note on the 0.62 upgrade helper.

cc @pvinis

Another good place would be adding a comment in the block comment in android/build.gradle (file) above:

project.ext.react = [
    enableHermes: false,  // clean and rebuild if changing
]

@alloy
Copy link
Member

alloy commented Mar 30, 2020

Hmm, yeah we actually went back and forth on this in the CHANGELOG react-native-community/releases#166 (comment) but dropped the ball in the end.

Thanks for the reminder, added this react-native-community/releases#182

alloy added a commit to react-native-community/releases that referenced this issue Mar 30, 2020
alloy added a commit to react-native-community/releases that referenced this issue Mar 30, 2020
@kelset
Copy link
Member

kelset commented Mar 30, 2020

Eloy's PR is now merged, I guess we can close this? :)

@lucasbento
Copy link
Member

We still have some work to do on Upgrade-Helper side, will be done today

@lucasbento
Copy link
Member

Added a comment to Upgrade-Helper to mention this change at react-native-community/upgrade-helper#224 and it should be up in prod in a few minutes.

Thank you so much for reporting this @muclipse, this will probably help a lot of people!

Thanks for the help here @pvinis & @alloy 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants