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

Allow annotating external module dependencies for fine-grained opt-out from compile checks. #95

Closed
Jadarma opened this issue Oct 21, 2023 · 10 comments
Labels
status:accepted Ticket is accepted and will be planned in a milestone type:improvement type:proposal
Milestone

Comments

@Jadarma
Copy link

Jadarma commented Oct 21, 2023

Is your feature request related to a problem? Please describe.
When enabling the KOIN_CONFIG_CHECK flag, the compiler will check the modules for completeness at compile time, which is great. However, I have encountered a gotcha when working with Ktor.

I would like to be able to build dependencies based on the application config, such as a database connection:

@Module
class DatabaseModule {

    @Single(createdAtStart = true)
    fun database(config: ApplicationConfig): Database = Database.connect(config.property("db.url").getString()
}

Since Ktor applications are scoped, there is no place to get the config from globally (and would also make it painful to test), so I register the config at the time of installing the Koin feature and have a reference to the environment:

fun Application.configureDI() {
    install(Koin) {
        modules(module(createdAtStart = true) {
            single { environment.config }
        })
        modules(DatabaseModule().module)
    }
}

This works perfectly at runtime, but the compilation now fails because KSP can't know that I will provide a separate module with the dependency at runtime or not.

Describe the solution you'd like
I do not want to disable the entire KSP checks for this one workaround. Instead, I would like to have an annotation that allows me to opt out of a check for a particular inject, and that it should just assume that it is available when validating the rest of the module.

This would document in code that I intentionally defined an external dependency and I am aware that I am responsible for ensuring its runtime availability, something like:

@Module
class DatabaseModule {

    @Single(createdAtStart = true)
    fun database(@External config: ApplicationConfig): Database = Database.connect(config.property("db.url").getString()
}

Describe alternatives you've considered
I have tried to see if it is possible to pass the config as a constructor parameter to the module, but code generation does not like it, and it makes sense since that would break the @Module(includes = []) feature, which expects a class reference with a no-args constructor.

I also tried to include the config module into the database module programatically, and while it works at runtime, it is again too late because the issue happens at compile time.

Target Koin project
Koin Annotations

@arnaudgiuliani arnaudgiuliani added status:checking Ticket is currently being checked type:proposal labels Jan 30, 2024
@arnaudgiuliani
Copy link
Member

Still wondering why you can't access a definition that would be included in your Koin ktor app module?

Or is it more here a problem of mixing Annotated/DSL config?

@Jadarma
Copy link
Author

Jadarma commented Jan 30, 2024

is it more here a problem of mixing Annotated/DSL config?

Yes.

I can access the definition at runtime, and everything works as expected from a DI point of view.
The issue is static compile checks enabled by KOIN_CONFIG_CHECK only scan in the context of a @Module and since I cannot get an instance of Ktor's ApplicationConfig from a global context, only from an instance of Application, I cannot register it directly in the module and must therefore provide it via the DSL, where I do have access to it.

While this mixing of annotations and DSL would work at runtime, the KOIN_CONFIG_CHECK does not know when, where, how, and if I will provide a dependency via the DSL and correctly reports it, failing the compilation.
However, that leaves me no workaround other than to disable the checks entirely, which is why I proposed the @External or similar mechanism to allow me to tell the plugin: "Yes, I know this might fail at runtime, but I explicitly made sure to configure it in the DSL, treat this as a false positive", while still having it check everything else for actual user error.

@arnaudgiuliani
Copy link
Member

ok, something that would be "ignored" for safety check

@cmdjulian
Copy link

I have the same problem :(

@steve1rm
Copy link

I am having a similar issue here #126

@arnaudgiuliani arnaudgiuliani added status:accepted Ticket is accepted and will be planned in a milestone and removed status:checking Ticket is currently being checked labels Jun 25, 2024
@arnaudgiuliani
Copy link
Member

the global idea is accepted. Don't know yet if we can call it @External or something else. I keep you posted.

@igorwojda
Copy link

Same issue here - It would be good to have a way to exclude certain deps from compiler checks

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Jul 12, 2024

In the compiler we have a notion of "external dependencies" now, that we can scan across multiple Gradle module. See #135.

I propose @Provided annotation. I'm afraid it confuses with Dagger's @provided. What do you think?
Else @Declared that would resonate more about something declared in Koin?

@arnaudgiuliani
Copy link
Member

Here is a PR with @Provided type: #140

@gabrielbmoro
Copy link

@arnaudgiuliani , I updated my personal repo to use the new annotation Provided, the weird thing is the compilation check is not behaving as expected with the newest version. I opened a PR to show this behavior -> https://github.com/gabrielbmoro/MovieDB-App/pull/162/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:accepted Ticket is accepted and will be planned in a milestone type:improvement type:proposal
Projects
None yet
Development

No branches or pull requests

6 participants