-
Notifications
You must be signed in to change notification settings - Fork 73
Use Context rather than Application #1284
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1284 +/- ##
==========================================
- Coverage 63.59% 63.56% -0.03%
==========================================
Files 142 142
Lines 3046 3049 +3
Branches 302 303 +1
==========================================
+ Hits 1937 1938 +1
- Misses 1032 1033 +1
- Partials 77 78 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
val ctx = | ||
when (context) { | ||
is Application -> context | ||
else -> context.applicationContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else -> context.applicationContext | |
else -> context.applicationContext ?: context |
applicationContext can be nullable in very specific cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marandaneto out of curiosity, what are those specific cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you init the SDK from a very specific lifecycle that is before the applicationContext
is fully initialized.
i dont recall the name of the lifeyccle method, but the context instance type is ContextImpl
and then the applicationContext
is still null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found it, if you init the SDK from attachBaseContext
, this will happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this relates to my comment here, where I was suggesting using Context.getApplicationContext()
for cases where we might need an Application-specific functionality, such as listening to activities' lifecycle callbacks.
The fact that there's a risk of users initializing the agent using a Context object that won't provide the application context could mean that some specific features won't be able to initialize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it is reasonable to document that as a caveat of this approach, specifically that if you attempt to initialize within attachBaseContext
that lifecycle callbacks won't work. At Embrace we cast applicationContext to Application
when setting lifecycle callbacks & anecdotally haven't experienced customers running into this issue. I do know that some folks want to initialize their SDK earlier than the Application
lifecycle and I view it as a tradeoff that allows them to capture early telemetry that might be more important to them than other telemetry that might become harder to capture because of the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on documenting but there's no reason not to fallback to context
if applicationContext
is null
if later on ctx
isnt castable to Application
, you just skip installing the features that wont work without it, thats how i've done many times before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current approach will assign null
to ctx
and then you cannot do almost anything if the SDK is init in the eg attachBaseContext
lifecycle, so no telemetry at all.
Goal
Uses
Context
rather thanApplication
for initializing the SDK as originally discussed in #1257.Context
is a less specific type thanApplication
which allows folks to initialize the SDK from places other than theApplication
subclass. Potential use-cases include:I do think that initializing from an
Application
subclass remains a sensible default & should be the recommended installation approach, but usingContext
gives the option for users to initialize in those use-cases if they wish.