fix: UUI-814 fix crash for lateinit instance property #107
Conversation
fix crash for lateinit instance property
pklawikowski-schibsted
requested review from
xserxses,
bogdan-niculescu-sch,
MarcinGie and
PiotrDabrowskiSTP
There was a problem hiding this comment.
Would still appreciate if someone having more experience (@bogdan-niculescu-sch ) could take a look at the PR too before merging 🙇
There was a problem hiding this comment.
I understand this will fix a series of crashes, and approach will most likely work as intended.
I have a few slight concerns of side-effects of these changes for the internal structure of the sdk, as well as adoption for developers since they will be breaking changes.
I also don't oppose the changes, but let me know what you think 👍
| @@ -91,7 +92,12 @@ class AuthResultLiveData private constructor(private val client: Client) : | |||
| if (::instance.isInitialized) instance else null | |||
|
|
|||
| @JvmStatic | |||
| fun get(): AuthResultLiveData = instance | |||
| fun get(client: Client): AuthResultLiveData { | |||
There was a problem hiding this comment.
hey, this change will be considered a breaking change right? since get will now require a mandatory client param
There was a problem hiding this comment.
i think this approach will also strongly link AuthResultLiveData to Client? i think currently AuthResultLiveData has indirect dependency to Client through AuthorizationManagementActivity
I am not sure but also this might allow app users to change used Auth client during app lifecycle if client is missing
There was a problem hiding this comment.
hey, this change will be considered a breaking change right? since get will now require a mandatory client param
Basically no, because the only place where devs are passing client inside SDK logic to configure itself is via AuthorizationManagementActivity.setup
i think this approach will also strongly link AuthResultLiveData to Client? i think currently AuthResultLiveData has indirect dependency to Client through AuthorizationManagementActivity
I am not sure but also this might allow app users to change used Auth client during app lifecycle if client is missing
From what I see, places where we are using AuthResultLiveData.get() are leveraged only in OnResume flow, which is totally internal logic.
AuthResultLiveData.getIfInitialised() is a safe getter used in User class.
I guess we dont want to allow users to use nullable variables, to maybe lets change the visibility of get(client) to prevent brands using non-safe method? If they really really need the AuthLiveData which is served through User client I dont see a reason to expose the get(client) method and make only an internal use?
Fix lateinit property by saving the client settings passed during
setupAdded some test to backup that solution is working