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

chore: extract classes needed to be shared #408

Merged
merged 16 commits into from
Nov 27, 2023

Conversation

Shahroz16
Copy link
Contributor

@Shahroz16 Shahroz16 commented Nov 20, 2023

Third draft PR for [CDP] iOS Refactor: Base module#11645

Problem statement:

Current DIGraph is bound by sdkConfig which is bound by SDK initialization
Push and in-app should be able to access classes without being dependent on SDK being initialized
Push and in-app should be able to access classes that are not bound by the tracking module initialing the SDK
Removed the dependency on SdkInitializedUtil We are holding a strong reference to digraph just for this purpose

    // The 1 place that DiGraph is strongly stored in memory for the SDK.
    // Exposed for `SdkInitializedUtil`. Not recommended to use this property directly.
    var diGraph: DIGraph?

Benefits:

Breaking sdkConfig to not be sandboxed and to be to utilize digraph would be a scope creep and a lot bigger change and this separate class removes that.
We have now added a concept of scoped graphs, previously everything was bound to sdkConfig scope hence the issue. Now we have a digraph that is not bound to anything, and we have a graph bound to sdkConfig and a template to add more graphs if needed for each module for the classes in that module
Events/Journeys needs to be shared instance among all modules to make sure multiple instances aren't created and events are not missed.
This approach makes sure that even if end up deciding we are going to keep tracking code (separate ticket), digraph or any of the older implementations doesn't break

We aren't modifying the DiGraph but rather adding another one, independent of sdkConfig that push and in-app are utilizing.

Copy link

github-actions bot commented Nov 20, 2023

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


  • APN-UIKit: Build failed. See CI job logs to determine the issue and try re-building.
  • CocoaPods-FCM: shahroz/native-digraph (1701081869)

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 105 lines in your changes are missing coverage. Please review.

Comparison is base (624312f) 58.17% compared to head (db961f3) 57.04%.

❗ Current head db961f3 differs from pull request most recent head adc9f79. Consider uploading reports for the commit adc9f79 to get more accurate results

Files Patch % Lines
Sources/Common/Util/KeyValueStorage.swift 0.00% 49 Missing ⚠️
Sources/Common/Util/Log.swift 0.00% 29 Missing ⚠️
Sources/Common/Store/GlobalDataStore.swift 0.00% 24 Missing ⚠️
Sources/Common/DIGraphShared.swift 0.00% 2 Missing ⚠️
Sources/Common/DIManager.swift 95.45% 1 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##           shahroz/cdp-native-refactor-base     #408      +/-   ##
====================================================================
- Coverage                             58.17%   57.04%   -1.13%     
====================================================================
  Files                                   123      125       +2     
  Lines                                  4727     4831     +104     
====================================================================
+ Hits                                   2750     2756       +6     
- Misses                                 1977     2075      +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@levibostian levibostian left a comment

Choose a reason for hiding this comment

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

I have some questions regarding this PR. I'll ask in Slack.

@Shahroz16 Shahroz16 changed the base branch from shahroz/cdp-native-refactor-base to shahroz/cdp-native-refactor-customerio-class November 21, 2023 14:44
Base automatically changed from shahroz/cdp-native-refactor-customerio-class to shahroz/cdp-native-refactor-base November 22, 2023 06:04
@Shahroz16 Shahroz16 marked this pull request as ready for review November 27, 2023 11:16
Copy link
Contributor

@mrehan27 mrehan27 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I can see this has been resolved in Slack, so looks like we are good merging this.

@Shahroz16 Shahroz16 merged commit d90ae74 into shahroz/cdp-native-refactor-base Nov 27, 2023
5 of 7 checks passed
@Shahroz16 Shahroz16 deleted the shahroz/native-digraph branch November 27, 2023 11: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.

3 participants