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.

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

Fix DeclaredScopedInstance into adapted ScopedInstanceFactory #2111

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

arnaudgiuliani
Copy link
Member

@arnaudgiuliani arnaudgiuliani commented Jan 8, 2025

Fix DeclaredScopedInstance solution to provide instance declared in rework previous with ScopedInstanceFactory

  • design ScopedInstanceFactory to allow to not hold instance function, when declared on the fly
  • added tests

… scope directly, and avoid leak the initial value
@arnaudgiuliani arnaudgiuliani added this to the 4.0.2 milestone Jan 8, 2025
@arnaudgiuliani arnaudgiuliani merged commit 67b8150 into 4.0.2 Jan 8, 2025
8 checks passed
@arnaudgiuliani arnaudgiuliani deleted the fix_declared_scope_instance branch January 8, 2025 13:58

@PublishedApi
internal fun saveValue(id : ScopeID, value : T){
values[id] = value
Copy link

Choose a reason for hiding this comment

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

This accessor is not thread safe

}
}
return values[context.scope.id] ?: error("Scoped instance not found for ${context.scope.id} in $beanDefinition")
return values[context.scope.id] ?: throw MissingScopeValueException("Factory.get -Scoped instance not found for ${context.scope.id} in $beanDefinition")
Copy link

Choose a reason for hiding this comment

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

This is a breaking change. Make sure you increase major version before release

Copy link
Member Author

Choose a reason for hiding this comment

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

not in that case, as with such new behavior we would have ISE and can't use getOrNull. Here external behavior is the same for the user, "just" a side effect internally to handle that case

Copy link

Choose a reason for hiding this comment

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

Then this api, including exception, should be internal

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, it is still an internal API. I can tag them @KoinInternalApi in 4.1 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants