Refactor retrieving advertisement id to Coroutines #93
Conversation
Such flow makes code easier to be extracted
Introducing an interface, allowed to create `FakeDeviceInfoRepository`.
There's no need to differentiate between different type of exceptions.
It was not used. Also, it's not expected that the mobile sdk will send this field.
As `DeviceInfoRepository` is now faked, the test doesn't have to use Robolectric anymore.
It encapsulates all logic related to retrieving an ad key
It encapsulates all logic related to retrieving uuid
Actually, it was never working as we never were saving any value to those `SharedPreferences`. This change seems fine because we rely in vast majority of cases on AdvertisingId - only in case of lack of Google Play Services we fallback to `ANDROID_ID` which is the same until factory reset. Closes: #59
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## coroutines #93 +/- ##
==============================================
+ Coverage 67.67% 68.24% +0.57%
==============================================
Files 14 16 +2
Lines 365 359 -6
Branches 52 52
==============================================
- Hits 247 245 -2
+ Misses 99 97 -2
+ Partials 19 17 -2 ☔ View full report in Codecov by Sentry. |
wzieba
changed the title
parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt
Outdated
Show resolved
Hide resolved
parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt
Show resolved
Hide resolved
| import kotlinx.coroutines.CoroutineScope | ||
| import kotlinx.coroutines.launch | ||
|
|
||
| internal class AdvertisementIdProvider( |
There was a problem hiding this comment.
I am curious about the general design choice for this class.
If I am understanding it correctly, when an object of this class is instantiated, if we were to read the adKey immediately, it'll return null. Looking at the constructor signature, there is a hint that we are using coroutines for something but there isn't much context about what it's used for unless we look into the implementation. So, I think a developer may not realize that gotcha and end up requesting the adKey before it's ready. Also, I am not even sure if there is a straightforward way to tell when the adKey is ready to be retrieved.
So, I was wondering if we should use a suspend function for provide so that it can be used in a proper coroutine scope without worrying about the internal details. What do you think?
There was a problem hiding this comment.
If I am understanding it correctly, when an object of this class is instantiated, if we were to read the adKey immediately, it'll return
null.
That's correct and intended 👍
Also, I am not even sure if there is a straightforward way to tell when the
adKeyis ready to be retrieved.
Yes, I don't see a way in current implementation. One suggestion would be to maybe use a (State)Flow? But I'm not sure if it wouldn't be an overkill for this case.
So, I was wondering if we should use a
suspendfunction forprovideso that it can be used in a proper coroutine scope without worrying about the internal details. What do you think?
We could do this, I was also experimenting with such design. My decision to go with not using suspend is the actual usage of AdvertisementIdProvider in the codebase. If we made suspend provide method, we'd have to move executing coroutine invocation higher in a chain of execution - maybe run coroutine in AndroidDeviceInfoRepository#collectDeviceInfo or EventsBuilder#buildEvent? It got all more complex than what I intended with such PR, so I decided to run coroutine in constructor of AdvertisementIdProvider and return null if the coroutine did not yet finish.
WDYT? I understand the possibly misleading design, but having the context above - would it make sense to stick to such design, or we should iterate? If we should iterate and make suspend provide - where would you suggest running actual coroutine?
There was a problem hiding this comment.
I think this one is important to get right. We don't have to use a suspended function if it's going to be more trouble than it's worth - but at the very least, I think it's worth having a big disclaimer in both the class definition and where it's consumed. The reason I strongly feel this way is because if we have any issues related to this, it can be hard to debug if the developer doesn't have any prior context. To be fair - the fact that it's marked as nullable may give enough context, so maybe it's not as big of a deal as I am making it out to be 🤷♂️
I don't think we should make any major changes in this PR, because it'll get hard to review it. However, if I was working on this, I'd give using suspended function a chance as a separate PR. Assuming we keep the current architecture, this will cause a chain reaction and the following methods may also need to become suspended functions: AndroidDeviceInfoRepository.collectDeviceInfo, EventsBuilder.buildEvent & ParselyTracker.trackPlay.
ParselyTracker.trackPlay: Looking at the name, I assumed that this function would eventually become a suspended function regardless of the decision we'll make here, but looking at the implementation, I am not so sure. I'd love to hear your thoughts!EventsBuilder.buildEvent: This probably shouldn't be a suspended function, but instead take the device info as a parameter. This is used inParselyTracker.trackPlay&ParselyTracker.trackPageviewboth of which could be suspended functions, so I think that'll work out.AndroidDeviceInfoRepository.collectDeviceInfo: Making this a suspended function makes sense to me. It communicates that collecting device info is not an immediate thing which could be for many reasons. We could be reading from file, checking a DB etc to collect this info. Admittedly some of the info is available immediately, so I could see an argument for keeping it a regular function as well. 🤷♂️
I think the decision will heavily depend on your vision for the SDK in general and I don't want to influence that vision too much. If you think it's better to leave the current design as is, I can live with that as long as we document it.
Hope you find my thoughts useful - let me know what you think!
There was a problem hiding this comment.
if I was working on this, I'd give using suspended function a chance as a separate PR
Gotcha 👍 I've created an issue to track it: #97
ParselyTracker.trackPlay: Looking at the name, I assumed that this function would eventually become a suspended function
This function is one of the few main exposed to the consumers of the SDK. I'd argue to not make it, or rest of the tracking methods (startEngagement, trackPageview) suspend, as it would force clients to use coroutines when using our API. It'd be problematic for them, and sometimes very complex, if the client's codebase is in Java.
AndroidDeviceInfoRepository.collectDeviceInfo: Making this a suspended function makes sense to me. It communicates that collecting device info is not an immediate thing which could be for many reasons
True, that'd be a good place. Still, we would have to find a way to wait for collectDeviceInfo to finish without blocking the SDK from accepting new events - I'd prefer to never allow user to wait for accepting a new event.
I think the decision will heavily depend on your vision for the SDK in general
That's true - I think it'd be best to combine this improvement with changes described in #94 ! Thank you for sharing your thoughts and suggestions - I really appreciate it as, even if we won't apply them right away, they'll be certainly useful during later phases.
I don't think we should make any major changes in this PR, because it'll get hard to review it.
As that's the case - would you mind accepting this PR, or do you think we should iterate on something more here?
There was a problem hiding this comment.
As that's the case - would you mind accepting this PR, or do you think we should iterate on something more here?
I just wanted this conversation to resolve before approving the PR - in case you wanted to add a comment about this issue. Approved now - thanks for the discussion!
There was a problem hiding this comment.
Ah right, thanks! Added a comment: 0c3102d. Merging 👍
parsely/src/test/java/com/parsely/parselyandroid/AndroidDeviceInfoRepositoryTest.kt
Outdated
Show resolved
Hide resolved
It's probably better to review this PR after #95 .
Description
This PR moves logic from
GetAdKey: AsyncTaskinto Coroutines-basedAdvertisementIdProvider.It also refactors and unifies getting another id:
ANDROID_ID, which should be used if the device is missing Google Play Services.How to test
Added and improved unit tests in this PR should be just fine in validating the introduced change.