Create SR module without native implementation #541

louiszawadzki · 2023-10-25T11:37:20Z

What does this PR do?

This creates a module for SR with no actual logic on the native side.
The configuration on the JS side is done, no more thing should be needed here after.

The package cannot be published to npm, even by accident.

Additional Notes

A coming PR will enable SR on both Android and iOS and add test + linting on native code in Bitrise.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
If this PR is auto-generated, please make sure also to manually update the code related to the change

louiszawadzki · 2023-10-25T11:39:18Z

packages/react-native-session-replay/android/build.gradle

+  buildToolsVersion getExtOrDefault('buildToolsVersion')
+  def agpVersion = com.android.Version.ANDROID_GRADLE_PLUGIN_VERSION
+  if (agpVersion.tokenize('.')[0].toInteger() >= 7) {
+    namespace = "com.datadog.sessionreplay.reactnative"


Let me know if you think we can come up with a better package name (the core package name is com.datadog.reactnative

yes, let's flip the parts and call it com.datadog.reactnative.sessionreplay, this will align with the naming on the native side, which is com.datadog.android.sessionreplay

louiszawadzki · 2023-10-25T16:11:47Z

packages/react-native-session-replay/package.json

+    },
+    "license": "Apache-2.0",
+    "main": "lib/commonjs/index",
+    "private": true,


this is the line that prevents the package from being published

buraizu

Approving the packages/react-native-session-replay/README.md update, but just wanted to ask whether it's necessary to add a page for something that's not supported yet. Is this something that's expected later?

I might be misunderstanding something or not have all the relevant context, but flagging just in case.

The merge-base changed after approval.

0xnm · 2023-10-26T07:46:23Z

packages/react-native-session-replay/android/.settings/org.eclipse.buildship.core.prefs

+connection.gradle.distribution=GRADLE_DISTRIBUTION(VERSION(6.0))
+connection.project.dir=
+eclipse.preferences.version=1
+gradle.user.home=
+java.home=/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home


is this file needed? it contains very old references to Gradle 6.0, which is not used for sure, and Java 11 is needed at least instead of the Java 1.8 referenced here.

0xnm · 2023-10-26T07:47:57Z

packages/react-native-session-replay/android/build.gradle

+  buildToolsVersion getExtOrDefault('buildToolsVersion')
+  def agpVersion = com.android.Version.ANDROID_GRADLE_PLUGIN_VERSION
+  if (agpVersion.tokenize('.')[0].toInteger() >= 7) {
+    namespace = "com.datadog.sessionreplay.reactnative"


yes, let's flip the parts and call it com.datadog.reactnative.sessionreplay, this will align with the naming on the native side, which is com.datadog.android.sessionreplay

0xnm · 2023-10-26T07:49:33Z

packages/react-native-session-replay/android/build.gradle

+  }
+}
+
+// This is needed to make unmock work. Choreographer class was loaded from android.jar of Android SDK


There is no more Choreographer class usage in Android SDK, is this configuration lambda needed?

0xnm · 2023-10-26T07:50:14Z

packages/react-native-session-replay/android/gradle.properties

+DatadogSDKReactNativeSessionReplay_compileSdkVersion=31
+DatadogSDKReactNativeSessionReplay_buildToolsVersion=31.0.0
+DatadogSDKReactNativeSessionReplay_targetSdkVersion=31


minor question: given that the current Android API is 34, should we target it?

I'd like to keep this in sync with the value used in the version of the RN dev dependency (currently 0.71).
So I agree it should be at least 33 which is the default version for RN 0.71 and 0.72.

Is it ok if I bump it to 34 once we migrate the RN dev dependency to 0.73?

My question is just theoretical, let's keep it in sync with the value used by the latest RN version we are using.

...droid/src/main/kotlin/com/datadog/sessionreplay/reactnative/DdSessionReplayImplementation.kt

...n-replay/android/src/newarch/kotlin/com/datadog/sessionreplay/reactnative/DdSessionReplay.kt

...-replay/android/src/test/kotlin/com/datadog/sessionreplay/reactnative/DdSessionReplayTest.kt

0xnm · 2023-10-26T09:15:32Z

packages/react-native-session-replay/android/build.gradle

+    api "com.facebook.react:react-android:$reactNativeVersion"
+  }
+  implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
+  compileOnly "com.squareup.okhttp3:okhttp:3.12.13"


FYI: SDK v2 is on OkHttp 4.11 https://github.com/DataDog/dd-sdk-android/blob/4744e9d7f85449708c96580ba5f7bd9695dbebc0/gradle/libs.versions.toml#L6, so you probably would like to change the version here.

Indeed it's not needed here.
We only use it for the ProxyAuthenticator and it's compileOnly, so the migration should be fairly easy for core.
I will create a technical task for the migration.

0xnm · 2023-10-26T09:16:46Z

...in/kotlin/com/datadog/sessionreplay/reactnative/DatadogSDKReactNativeSessionReplayPackage.kt

all files should be moved to conform the new namespace, they shouldn't be under com/datadog/sessionreplay/reactnative, but under com/datadog/reactnative/sessionreplay

0xnm · 2023-10-26T14:07:09Z

packages/react-native-session-replay/android/build.gradle

+    sourceCompatibility JavaVersion.VERSION_1_8
+    targetCompatibility JavaVersion.VERSION_1_8


SDK v2 is producing Java 11 bytecode, so maybe you would like to align

What is the risk with changing this value?
I would prefer to make the change in a dedicated PR to avoid breaking support for some client apps.

I guess there is no risk, because SDK v2 artifacts are already with Java 11 bytecode anyway. Out of curiosity, what bytecode level is emitted by RN itself?

It's VERSION_11 for the latest versions.

0xnm · 2023-10-26T14:07:53Z

packages/react-native-session-replay/android/build.gradle

+unMock {
+  keep("android.os.Looper")
+  keep("android.os.MessageQueue")
+  keep("android.os.SystemProperties")
+  keep("android.view.Choreographer")
+  keep("android.view.DisplayEventReceiver")
+}


not sure if all this is still needed with SDK v2

Not needed for RNSR but still needed for core tests. I will remove in the next PR so you don't have to approve this one again for it to be merged :)

louiszawadzki · 2023-10-26T15:27:47Z

@buraizu I wanted to add an indication in case someone noticed it browsing github and would attempt to use it right away.

0xnm · 2023-10-27T12:11:22Z

packages/react-native-session-replay/android/build.gradle

+  testImplementation "com.github.xgouchet.Elmyr:jvm:1.3.1"
+  testImplementation "com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0"
+  testImplementation "org.jetbrains.kotlin:kotlin-reflect:$kotlin_version"
+  unmock 'org.robolectric:android-all:4.4_r1-robolectric-r2'


probably you don't need this either and no need to apply unmock plugin as well, but this is very minor, can be addressed later

louiszawadzki requested review from a team as code owners October 25, 2023 11:37

louiszawadzki commented Oct 25, 2023

View reviewed changes

louiszawadzki force-pushed the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch from 728336e to 85ee611 Compare October 25, 2023 11:53

louiszawadzki mentioned this pull request Oct 25, 2023

Fix webview telemetry for new architecture apps #542

Merged

4 tasks

louiszawadzki force-pushed the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch 5 times, most recently from d4fc364 to 5f9f7eb Compare October 25, 2023 14:11

louiszawadzki commented Oct 25, 2023

View reviewed changes

buraizu previously approved these changes Oct 25, 2023

View reviewed changes

0xnm reviewed Oct 26, 2023

View reviewed changes

louiszawadzki added 2 commits October 26, 2023 10:24

Remove old android settings files

10898f9

Update sdk versions

f06804b

louiszawadzki force-pushed the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch 2 times, most recently from 1697817 to 5c54e0d Compare October 26, 2023 08:50

louiszawadzki requested a review from 0xnm October 26, 2023 09:05

louiszawadzki force-pushed the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch from 5c54e0d to 0f14db3 Compare October 26, 2023 09:07

0xnm reviewed Oct 26, 2023

View reviewed changes

louiszawadzki force-pushed the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch 2 times, most recently from 1765a47 to a65de6b Compare October 26, 2023 13:43

louiszawadzki requested a review from 0xnm October 26, 2023 13:45

0xnm previously approved these changes Oct 26, 2023

View reviewed changes

louiszawadzki added 4 commits October 27, 2023 14:05

Update java compatibility

3406317

Create Session Replay module with no option

48d56d5

Move Session Replay to own file

7e41a5a

Add js test for session replay

a6a53c6

louiszawadzki added 3 commits October 27, 2023 14:05

Add configuration in JS for Session Replay

fe287d9

Pass configuration to native code

e9746e2

Run tests for each package in isolation for mocks

5c58db4

louiszawadzki dismissed 0xnm’s stale review via 5c58db4 October 27, 2023 12:05

louiszawadzki force-pushed the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch from a65de6b to 5c58db4 Compare October 27, 2023 12:05

0xnm approved these changes Oct 27, 2023

View reviewed changes

louiszawadzki merged commit 62c2833 into develop Oct 27, 2023
3 checks passed

louiszawadzki deleted the louiszawadzki/rum-1815/create-sr-module-without-native-implementation branch October 27, 2023 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create SR module without native implementation #541

Create SR module without native implementation #541

louiszawadzki commented Oct 25, 2023

louiszawadzki Oct 25, 2023

0xnm Oct 26, 2023

louiszawadzki Oct 25, 2023

buraizu left a comment

0xnm Oct 26, 2023

0xnm Oct 26, 2023

0xnm Oct 26, 2023

0xnm Oct 26, 2023

louiszawadzki Oct 26, 2023

0xnm Oct 26, 2023

0xnm Oct 26, 2023

louiszawadzki Oct 26, 2023

0xnm Oct 26, 2023

0xnm Oct 26, 2023

louiszawadzki Oct 27, 2023

0xnm Oct 27, 2023 •

edited

Loading

louiszawadzki Oct 27, 2023

0xnm Oct 26, 2023

louiszawadzki Oct 27, 2023

louiszawadzki commented Oct 26, 2023 •

edited

Loading

0xnm Oct 27, 2023

		sourceCompatibility JavaVersion.VERSION_1_8
		targetCompatibility JavaVersion.VERSION_1_8

Create SR module without native implementation #541

Create SR module without native implementation #541

Conversation

louiszawadzki commented Oct 25, 2023

What does this PR do?

Additional Notes

Review checklist (to be filled by reviewers)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buraizu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnm Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louiszawadzki commented Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

0xnm Oct 27, 2023 •

edited

Loading

louiszawadzki commented Oct 26, 2023 •

edited

Loading