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?
pklawikowski-schibsted
deleted the
feature/UUI-814-fix-auth-result-crash
branch
Fix lateinit property by saving the client settings passed during
setupAdded some test to backup that solution is working