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

Add method to notify CodePush update into RN internal interface. #496

Merged
merged 4 commits into from
Mar 4, 2024

Conversation

lucaslabari
Copy link
Contributor

Goal

  • From RN side we have a way to know if CodePush has recently updated the bundle.
  • In this PR I added a method to differentiate CodePush from other OTAs.
  • If a customer is using CodePush, RN SDK will call setJavaScriptBundleUrlForCodePush passing also a boolean indicating if this is the first run for that bundle and we need to update the bundleId.
  • If a customer is using other OTA like Expo, RN SDK will call setJavaScriptBundleUrl and the bundleId will need to be calculated each time this method is called with a new bundleUrl, because if it's the same as the persisted one, the bundleId was calculated at startup.
  • At startup, we calculate the bundleId if it's not persisted and if the bundleURL is not persisted either, we use buildInfo.buildId as the default value.

Testing

Added unit tests.

Release Notes

  • Optimized Bundle Id calculation for RN Apps using CodePush.

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

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

Project coverage is 79.49%. Comparing base (f31bbb3) to head (d74b3aa).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
- Coverage   79.54%   79.49%   -0.06%     
==========================================
  Files         379      380       +1     
  Lines       10288    10300      +12     
  Branches     1507     1509       +2     
==========================================
+ Hits         8184     8188       +4     
- Misses       1513     1517       +4     
- Partials      591      595       +4     
Files Coverage Δ
...oid/embracesdk/capture/metadata/MetadataService.kt 100.00% <100.00%> (ø)
...roid/embracesdk/prefs/EmbracePreferencesService.kt 93.42% <100.00%> (+0.08%) ⬆️
...ace/android/embracesdk/prefs/PreferencesService.kt 0.00% <ø> (ø)
...oid/embracesdk/ReactNativeInternalInterfaceImpl.kt 62.22% <90.00%> (+0.31%) ⬆️
...racesdk/capture/metadata/EmbraceMetadataService.kt 87.88% <83.33%> (-0.37%) ⬇️

... and 2 files with indirect coverage changes

@fnewberg
Copy link

fnewberg commented Mar 4, 2024

@lucaslabari what is the reason we are making this change? was the old approach causing ANRs?

@lucaslabari
Copy link
Contributor Author

@lucaslabari what is the reason we are making this change? was the old approach causing ANRs?

The ANRs were solved in another PR. The goal of this PR is to optimize the calculation of the RN bundleId that is sometimes a heavy operation. We are not blocking the main thread anymore, but if we calculate the bundleId each time and the bundle is a heavy file, then the early events could go to the backend with a null value for the bundleId in the payload. So the idea is to minimize this given that we have a way to know if CodePush updated the bundle.

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

A couple of tweak suggestions but looks good overall

@lucaslabari lucaslabari merged commit 5b7f82c into master Mar 4, 2024
4 checks passed
@lucaslabari lucaslabari deleted the lucas/set_bundle_id_code_push branch March 4, 2024 19:27
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.

3 participants