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

Hermes is working #2

Merged

Conversation

sentryadam0000345
Copy link
Contributor

@sentryadam0000345 sentryadam0000345 commented Apr 21, 2021

Hermes related changes that need to be done if you wish to enable it, if done, there is some text in the UI that just says Hermes is enabled and your events will have a Hermes tag attached.

Event:

(https://sentry.io/organizations/testorg-az/issues/2334943321/events/e54b5bf1e0e6481c90f013d97b95d0e1/?project=5713490&query=is%3Aunresolved)

Hermes Version Used:

hermes-engine: "0.5.0"

hermes-profile-transformer: "0.0.6"

Problem/Solution/ExpectedOutcome:

See Readme.

```
emulator -list-avds

emulator @{YourEmulator} -dns-server 8.8.8.8
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, are these the emulator commands you ended up using? Did you launch an emulator from CLI?

Or do you use the AVD through Android Studio?

if you could describe your workflow on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

README.md Outdated

Android

In your build.gradle file make sure to have the
Copy link
Contributor

Choose a reason for hiding this comment

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

is all of this already in the build.gradle file? If so, we don't have to repeat it all in the readme here.

Update - I checked, I see it in your /android/app/build.gradle.

My request - can you write "see /android/app/build.gradle" here under "To enable a Hermes event"?

Then in android/app/build.gradle can you put a comment somewhere like

// This is for enabling Hermes

I'm curious about the "androidx.swiperefreshlayout:swiperefreshlayout:1.0.0" and "facebook.flipper" things that I see, if those are for Hermes then put "// This is for Hermes" as it's not clear what swiperefresh and flipper are for.

If those are for some UI behaviors (non-Hermes) then put a comment clarifying what they're for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a piece of documentation/tutorial/blog you worked off of for this? If you still can find it, if you could paste the link into the PR that'd be great. It's helpful if we're ever debugging Hermes stuff in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -184,7 +189,7 @@ android {
dependencies {
implementation fileTree(dir: "libs", include: ["*.jar"])
//noinspection GradleDynamicVersion
implementation "com.facebook.react:react-native:+" // From node_modules
implementation "com.facebook.react:react-native:0.64.0" // From node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you indicate in the README (and PR would be nice too) what the current supported version of React Native is for our demo? Nice job with getting to 0.64.0 as that's the latest.

I know it was missing previously but we're slowly updating all our demo's so the framework/platform version we're building on is always indicated. Mobile demo's are very sensitive to changes in this too so it's good to document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -217,4 +222,31 @@ task copyDownloadableDepsToLibs(type: Copy) {
into 'libs'
}

sentry {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this config section here for Hermes?

Or is this for general io.sentry.android.gradle? Can you notate in the PR description that you added io.sentry.android.gradle? That's a really significant and awesome piece of work you added which you should note in the PR, and I'd even document in the README that this demo now has io.sentry.android.gradle plugin 👏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was in general for io.sentry.android.gradle but was left there on accident. Sorry to get you excited. I removed this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-6.2-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-6.7-all.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put in the README that we're on gradle 6.7?

A lot of times when people clone our mobile demo's and get errors when running, it's because they're not on the right versions of Gradle, so it's good to know what was last worked with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, done

@@ -40,7 +40,7 @@ if defined JAVA_HOME goto findJavaFromJavaHome

set JAVA_EXE=java.exe
%JAVA_EXE% -version >NUL 2>&1
if "%ERRORLEVEL%" == "0" goto init
if "%ERRORLEVEL%" == "0" goto execute
Copy link
Contributor

Choose a reason for hiding this comment

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

Did your changes in android/gradlew.bat occur here as a result of building with a new version of Gradle in AndroidStudio?

If not, could you link me to the documentation you followed for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I did not make any direct changes to android/gradlew.bat. These changes probably came from a new version of Gradle.

"@react-native-community/masked-view": "^0.1.10",
"@react-navigation/native": "^5.7.3",
"@react-navigation/stack": "^5.9.0",
"@sentry/react-native": "2.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 are huge Adam nice work, please put this in your PR description as well as the README

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm talking about the sentry/react-native, react, and react-native here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Yup, done.

defaults.project=mobile-webinar
auth.token=XXXX
defaults.project=reactnativeadamtest
auth.token=bbece28fc4b847baaed54aa3518c5e7ced9bbaaac4404fc48f81dea11347caae
Copy link
Contributor

Choose a reason for hiding this comment

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

@sentryadam0000345 remove this Auth Token from source code. Then delete it from your Settings in Sentry.io and generate a new one and start using that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

defaults.project=mobile-webinar
auth.token=XXXX
defaults.project=adamreactnative
auth.token=bbece28fc4b847baaed54aa3518c5e7ced9bbaaac4404fc48f81dea11347caae
Copy link
Contributor

Choose a reason for hiding this comment

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

@sentryadam0000345 Remove this Auth Token from source code, then delete it from your Sentry.io settings and generate a new one and start using that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sentryadam0000345 sentryadam0000345 merged commit 53edb0e into sentry-demos:master May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants