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

[FC] Adds ActivityRetainedScope and broader singleton component #9821

Merged

Conversation

carlosmuvi-stripe
Copy link
Collaborator

@carlosmuvi-stripe carlosmuvi-stripe commented Dec 23, 2024

Summary

Today, we're using the @Singleton dagger scope wrongly, as dependencies are not shared across activities or match the app's lifecycle (see docs).

This PR replaces current Singleton scope usages by ActivityRetainedScope, that clearly defines the real scope of existing dependencies.

Additionally, this PR includes a way to share dependencies across activities (real @Singleton scope), by defining a thread-safe singleton pattern for SharedComponentHolder to manage shared dependencies (e.g., Play Integrity API) across activities.

Motivation

We're preparing the ground to share the Integrity component across activities.

The idea is to call Integrity#prepare on Activity A (FinancialConnectionsSheetActivity) and reuse the same Integrity instance in Activity B (FinancialConnectionsNativeActivity) when generating tokens.

Took the opportunity to fix our current scope setup.

Testing

  • Added tests
  • Modified tests
  • Manually verified

@carlosmuvi-stripe carlosmuvi-stripe changed the title Adds activityRetainedScope [FC] Adds activityRetainedScope and Shared singleton state Dec 23, 2024
@carlosmuvi-stripe carlosmuvi-stripe changed the title [FC] Adds activityRetainedScope and Shared singleton state [FC] Adds ActivityRetainedScope and broader singleton component Dec 23, 2024
@carlosmuvi-stripe carlosmuvi-stripe marked this pull request as ready for review December 23, 2024 12:18
@carlosmuvi-stripe carlosmuvi-stripe requested review from a team as code owners December 23, 2024 12:18
Copy link
Contributor

github-actions bot commented Dec 23, 2024

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 301.8 KiB │ 301.8 KiB │  0 B │ 455.5 KiB │ 455.5 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.2 KiB │  90.2 KiB │ +3 B │ 170.3 KiB │ 170.3 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ +3 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19971 │ 19971 │ 0 (+0 -0) 
   types │  6191 │  6191 │ 0 (+0 -0) 
 classes │  4982 │  4982 │ 0 (+0 -0) 
 methods │ 29772 │ 29772 │ 0 (+0 -0) 
  fields │ 17542 │ 17542 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3622 │ 3622 │  0
APK
   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 28.5 KiB │ +3 B │  62.9 KiB │  0 B │ ∆ META-INF/CERT.SF                        
 25.3 KiB │ +3 B │  62.8 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
  1.2 KiB │ -2 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA                       
    271 B │ -1 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 55.2 KiB │ +3 B │ 127.1 KiB │  0 B │ (total)

@carlosmuvi-stripe carlosmuvi-stripe force-pushed the carlosmuvi/12-21-adds-activity-retained-scope branch from 5c4c9af to cc8fb23 Compare December 23, 2024 14:22
}

@Module
internal class FinancialConnectionsSingletonSharedModule
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll be adding the integrity manager here later in this stack.

@carlosmuvi-stripe carlosmuvi-stripe force-pushed the carlosmuvi/12-21-adds-activity-retained-scope branch 2 times, most recently from fa975d8 to 81df1b9 Compare December 25, 2024 11:00
@carlosmuvi-stripe carlosmuvi-stripe force-pushed the carlosmuvi/12-21-adds-activity-retained-scope branch 3 times, most recently from c2335d0 to 18a1ac0 Compare January 9, 2025 01:39

@Module
internal object FinancialConnectionsSheetConfigurationModule {

@Provides
@Named(PUBLISHABLE_KEY)
@Singleton
@ActivityRetainedScope
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that, given the new component structure, there's no room for mistakenly scoping a dependency as @singleton on a non shared component. Dagger would fail at compile time if one of these (or other module) dependencies are scoped as @singleton.

tillh-stripe
tillh-stripe previously approved these changes Jan 10, 2025
internal interface FinancialConnectionsSingletonSharedComponent {

@Component.Builder
interface Builder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This could use Component.Factory.

@carlosmuvi-stripe carlosmuvi-stripe merged commit 3147184 into master Jan 10, 2025
14 checks passed
@carlosmuvi-stripe
Copy link
Collaborator Author

Merge activity

  • Jan 10, 12:33 PM EST: A user merged this pull request with Graphite.

@carlosmuvi-stripe carlosmuvi-stripe deleted the carlosmuvi/12-21-adds-activity-retained-scope branch January 10, 2025 17:33
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.

2 participants