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

RUM-7005: updating dependencies, adjusting detekt rules, fix tests #2463

Merged

Conversation

satween
Copy link
Contributor

@satween satween commented Dec 18, 2024

What does this PR do?

We’re updating androidx.navigation to 2.8.0. Our goal is to maintain a balance between using stable library versions and minimizing disruptive changes to our customers’ codebases.

Motivation

This update motivated by contributor request

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • 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)

@satween satween requested a review from xgouchet December 18, 2024 17:21
@satween satween force-pushed the tvaleev/rum-7005/updating-android-navigation-dependencies branch from ab4b2bc to 474a55b Compare December 18, 2024 19:00
Copy link
Member

@xgouchet xgouchet left a comment

Choose a reason for hiding this comment

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

Note

Nice work, one question I have is, do we need to upgrade other AndroidX dependencies?
As a rule of thumb, I'd suggest upgrading our dependencies to the other AndroidX dependencies to versions released around the same time as 2.7.7 (i.e.: February 2024).

@satween satween force-pushed the tvaleev/rum-7005/updating-android-navigation-dependencies branch 2 times, most recently from 97e5164 to c308ec1 Compare December 23, 2024 11:06
@satween
Copy link
Contributor Author

satween commented Dec 23, 2024

Note

Nice work, one question I have is, do we need to upgrade other AndroidX dependencies? As a rule of thumb, I'd suggest upgrading our dependencies to the other AndroidX dependencies to versions released around the same time as 2.7.7 (i.e.: February 2024).

I've updated rest of the android libs to be aligned to the navigation. navigation lib upgraded to 2.8.0 as it contains few leaks fixes

Comment on lines 36 to 37
org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 : 20 Kb
org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm:1.7.1 : 1512 Kb
Copy link
Member

Choose a reason for hiding this comment

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

what brings this to the list? lifecycle libs update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered above, many of androidx libs depends on that

Copy link
Member

Choose a reason for hiding this comment

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

looks like it is coming from work-runtime update originally in the core module?

Copy link
Contributor Author

@satween satween Dec 31, 2024

Choose a reason for hiding this comment

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

not only work-runtime, but also some of the androidx libs as well. As described below, seems like I found combination of libs that not brings coroutines as a dependency. But in a future updates they will be added anyway

@satween satween force-pushed the tvaleev/rum-7005/updating-android-navigation-dependencies branch from 25d5cab to 8ef274c Compare December 24, 2024 12:22
androidx.lifecycle:lifecycle-livedata-core:2.5.1 : 9 Kb
androidx.lifecycle:lifecycle-livedata:2.5.1 : 11 Kb
androidx.startup:startup-runtime:1.1.1 : 18 Kb
androidx.work:work-runtime:2.9.1 : 1795 Kb
Copy link
Member

Choose a reason for hiding this comment

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

So maybe we should stay with 2.8.1 to avoid bringing coroutines? or maybe we can exclude them if they are not used in our code paths?

This is just an idea, I'm curious to see more opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont want to exclude any of dependencies, because we could create unstable build this way. Rolled back all androidX libs that could add coroutines as a transitive dependency. But we will face this problem in a future anyway

Copy link
Member

Choose a reason for hiding this comment

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

yes, staying with work-runtime:2.8.1 seems safest, I don't see any fixes/changes that would impact our usage flows in 2.9.1.

Comment on lines 36 to 37
org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1 : 20 Kb
org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm:1.7.1 : 1512 Kb
Copy link
Member

Choose a reason for hiding this comment

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

looks like it is coming from work-runtime update originally in the core module?

@satween satween force-pushed the tvaleev/rum-7005/updating-android-navigation-dependencies branch from 8ef274c to 75a7c60 Compare December 31, 2024 04:18
@satween satween force-pushed the tvaleev/rum-7005/updating-android-navigation-dependencies branch from 75a7c60 to a5709c3 Compare December 31, 2024 12:57
@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 69.92%. Comparing base (c29bc1f) to head (a5709c3).
Report is 11 commits behind head on develop.

Files with missing lines Patch % Lines
...nal/tracking/AndroidXFragmentLifecycleCallbacks.kt 50.00% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2463      +/-   ##
===========================================
+ Coverage    69.91%   69.92%   +0.01%     
===========================================
  Files          787      787              
  Lines        29347    29349       +2     
  Branches      4895     4898       +3     
===========================================
+ Hits         20516    20520       +4     
  Misses        7470     7470              
+ Partials      1361     1359       -2     
Files with missing lines Coverage Δ
...lin/com/datadog/android/compose/InteractionType.kt 100.00% <100.00%> (ø)
...id/compose/internal/InternalInteractionTracking.kt 91.43% <100.00%> (-0.16%) ⬇️
...nal/tracking/AndroidXFragmentLifecycleCallbacks.kt 91.89% <50.00%> (-2.70%) ⬇️

... and 35 files with indirect coverage changes

@satween satween marked this pull request as ready for review December 31, 2024 14:25
@satween satween requested review from a team as code owners December 31, 2024 14:25
@satween satween requested a review from 0xnm December 31, 2024 14:25
androidXAnnotations = "1.9.1"
androidXAppCompat = "1.4.2" # Next version will bring coroutines
androidXCollection = "1.4.5"
androidXComposeBom = "2023.10.01" # Next version will bring coroutines
Copy link
Member

Choose a reason for hiding this comment

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

having coroutines as a part of Compose is fine I think, it is an essential part of the apps using Compose in most cases, so we can go higher here. The total size of transitive dependencies for dd-sdk-android-compose is big already anyway.

@satween satween merged commit cc84b32 into develop Jan 2, 2025
23 checks passed
@satween satween deleted the tvaleev/rum-7005/updating-android-navigation-dependencies branch January 2, 2025 10:18
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.

4 participants