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.

Sign up for GitHub

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

Jump to bottom

koin-compose: use rememberUpdatedState with ParametersDefinition, optimized KoinApplication, KoinContext, KoinIsolatedContext #1768

Merged
merged 11 commits into from
Apr 15, 2024
Merged

koin-compose: use rememberUpdatedState with ParametersDefinition, optimized KoinApplication, KoinContext, KoinIsolatedContext #1768

merged 11 commits into from
Apr 15, 2024

Conversation

hoc081098
Copy link
Contributor

@hoc081098 hoc081098 commented Jan 24, 2024 •
edited
Loading

  • use rememberUpdatedState with ParametersDefinition
  • delegate rememberKoinInject to koinInject, update deprecated message (will close Write Normal Deprecation Messages With Replacement #1743)
  • optimized KoinApplication, KoinContext, KoinIsolatedContext
  • remove unused UnknownKoinContext
  • apiDump

@hoc081098 hoc081098 mentioned this pull request Jan 25, 2024
Closed
@hoc081098 hoc081098 changed the title koinInject: use rememberUpdatedState with ParametersDefinition koinInject: use rememberUpdatedState with ParametersDefinition, optimized KoinApplication, KoinContext, KoinIsolatedContext Jan 25, 2024
@hoc081098 hoc081098 changed the title koinInject: use rememberUpdatedState with ParametersDefinition, optimized KoinApplication, KoinContext, KoinIsolatedContext koin-compose: use rememberUpdatedState with ParametersDefinition, optimized KoinApplication, KoinContext, KoinIsolatedContext Jan 25, 2024
@arnaudgiuliani
Copy link
Member

is it optimizing around rememberUpdatedState? What is the target of this PR?

@arnaudgiuliani arnaudgiuliani added compose status:checking currently in analysis - discussion or need more detailed specs labels Jan 25, 2024
@hoc081098
Copy link
Contributor Author

hoc081098 commented Jan 25, 2024 •
edited
Loading

is it optimizing around rememberUpdatedState? What is the target of this PR?

  • I have used rememberUpdatedState to make sure scope.get... will use the latest ParametersDefinition (but it does not cause unnecessary recomposition 🙏).

Previous code has an issue where scope.get(...) use the old ParametersDefinition, since it is remembered.

  • I have also refactored KoinContext, KoinApplication, ... Basically, I have replaced CompositionLocalProvider (...){ content() } with CompositionLocalProvider(..., content = content)
  • apiDump and some format changes as well

hoc081098
hoc081098 commented Jan 25, 2024
View reviewed changes
compose/koin-compose/src/commonMain/kotlin/org/koin/compose/KoinApplication.kt Outdated
Comment on lines 64 to 66
fun getKoin(): Koin = currentComposer.run {
return LocalKoinApplication.current
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currentComposer.run is unnecessary

Copy link
Member

Choose a reason for hiding this comment

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

yeah 🤔 I would need to benchmark this, being sure we don't miss something behind the scene

Copy link
Contributor Author

@hoc081098 hoc081098 Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@arnaudgiuliani
From AndroidX:

xxx.current === currentComposer.consume(xxx)

https://github.com/androidx/androidx/blob/6d1f16a70c802850a83daae8929ea1662f972715/compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/CompositionLocal.kt#L69-L75

@Stable
sealed class CompositionLocal<T> constructor(defaultFactory: () -> T) {
    internal val defaultValueHolder = LazyValueHolder(defaultFactory)

    internal abstract fun updatedStateOf(value: T, previous: State<T>?): State<T>

    /**
     * Return the value provided by the nearest [CompositionLocalProvider] component that invokes, directly or
     * indirectly, the composable function that uses this property.
     *
     * @sample androidx.compose.runtime.samples.consumeCompositionLocal
     */
    @OptIn(InternalComposeApi::class)
    inline val current: T
        @ReadOnlyComposable
        @Composable
        get() = currentComposer.consume(this)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add @ReadOnlyComposable to getKoin() will optimize the generated code.
Docs: https://developer.android.com/reference/kotlin/androidx/compose/runtime/ReadOnlyComposable

Copy link
Member

Choose a reason for hiding this comment

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

thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just fixed the conflict. Could you check this PR again 🙏 ?

compose/koin-compose/src/commonMain/kotlin/org/koin/compose/KoinApplication.kt Outdated
Comment on lines 75 to 77
fun currentKoinScope(): Scope = currentComposer.run {
return LocalKoinScope.current
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currentComposer.run is unnecessary

compose/koin-compose/src/commonMain/kotlin/org/koin/compose/KoinApplication.kt Outdated
logger.info("[Warning] - No Koin context defined in Compose, fallback to default Koin context.\nUse KoinContext(), KoinAndroidContext() or KoinApplication() to setup or create Koin context with Compose and avoid such message.")
logger.info("[Warning] - No Koin context defined in Compose, fallback to default Koin context." +
"Use KoinContext(), KoinAndroidContext() or KoinApplication() to setup or create Koin context with Compose and avoid such message.")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Format string to 2 lines to improve the readability

hoc081098
hoc081098 commented Jan 25, 2024
View reviewed changes
compose/koin-compose/src/commonMain/kotlin/org/koin/compose/Inject.kt Outdated
Comment on lines 59 to 65
): T {
val st = parameters?.let { rememberStableParametersDefinition(parameters) }
return remember(qualifier, scope) {
scope.get(qualifier, st?.parametersDefinition)
}
}
): T = koinInject(qualifier, scope, parameters)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

delegate to koinInject

hoc081098
hoc081098 commented Jan 25, 2024
View reviewed changes
compose/koin-compose/src/commonMain/kotlin/org/koin/compose/KoinApplication.kt Outdated
Comment on lines 124 to 126
LocalKoinScope provides koinApplication.koin.scopeRegistry.rootScope
) {
content()
}
LocalKoinScope provides koinApplication.koin.scopeRegistry.rootScope,
content = content
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reduce an unnecessary lambda allocation
{ content() } -> content=content

hoc081098 added a commit to hoc081098/kmp-viewmodel that referenced this pull request Feb 3, 2024
@hoc081098
koinKmpViewModel: use rememberUpdatedState with ParametersDefinition
8b8a24f 
Closes #99
Related InsertKoinIO/koin#1768
@hoc081098
local deps
2902664
@hoc081098
Merge branch 'master' of https://github.com/hoc081098/koin into fix-k…
a4f5f05 
…oinInject

# Conflicts:
#	compose/build.gradle
#	compose/gradle/wrapper/gradle-wrapper.properties
#	compose/koin-androidx-compose-navigation/build.gradle
#	compose/koin-androidx-compose/build.gradle
#	compose/koin-compose/build.gradle
#	projects/compose/koin-compose/src/commonMain/kotlin/org/koin/compose/error/UnknownKoinContext.kt
#	projects/compose/koin-compose/src/commonMain/kotlin/org/koin/compose/stable/StableHolders.kt
@hoc081098
update
0012bb3
@arnaudgiuliani arnaudgiuliani added this to the compose-3.6.0 milestone Feb 15, 2024
@arnaudgiuliani arnaudgiuliani changed the base branch from main to 3.6.0 February 15, 2024 16:18
@arnaudgiuliani
Copy link
Member

Good work. I need to take time to take a tour :)

@arnaudgiuliani
Copy link
Member

It's just a api diff confilct I believe @hoc081098 👍

@hoc081098
Copy link
Contributor Author

It's just a api diff confilct I believe @hoc081098 👍

I will update it 🙏

@arnaudgiuliani
Copy link
Member

done edit for you @hoc081098 👍

@arnaudgiuliani arnaudgiuliani merged commit 45da225 into InsertKoinIO:3.6.0 Apr 15, 2024
3 of 4 checks passed
@hoc081098
Copy link
Contributor Author

done edit for you @hoc081098 👍

Thanks for your help

@hoc081098 hoc081098 deleted the fix-koinInject branch April 16, 2024 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnaudgiuliani arnaudgiuliani arnaudgiuliani approved these changes

Assignees
No one assigned
Labels
compose status:checking currently in analysis - discussion or need more detailed specs
Projects
None yet
Milestone
compose-3.6.0
Development

Successfully merging this pull request may close these issues.

Write Normal Deprecation Messages With Replacement
2 participants
@hoc081098 @arnaudgiuliani