evant / kotlin-inject

Dependency injection lib for kotlin
Apache License 2.0
1.24k stars 55 forks source link

Add a new annotation to generate Component accessors for KMP targets #370

Closed eygraber closed 5 months ago

eygraber commented 6 months ago

Fixes #361 Fixes #371

eygraber commented 6 months ago

@evant naming is still not great, but I think it's kind of descriptive of what it is doing.

What do you think in terms of the module location, and moving the KSP test harness so that it could be used by both KSP modules?

evant commented 6 months ago

Oh one more thought, do we actually care about the companion here? Seems to me this is just a convenience in how the function is called, but

@TargetComponentAccessor
expect fun createMyCustomComponent(): MyCustomComponent

would work just as well. Id suggest blindly copying the function signature and not caring about if/what the receiver is.

eygraber commented 6 months ago

I was using the receiver to determine which create function to call, but I guess it makes more sense to use the return type. I also need to check if I should use the companion or KClass

evant commented 6 months ago

You can check me.tatarka.inject.generateCompanionExtensions to determine if you are generating kclass or companion calls

eygraber commented 6 months ago

Pushed all of those changes. I'll start writing tests tomorrow, but I did some refactoring to separate the Inject and TargetComponentAccessor processing, and wanted to get your thoughts on that.

eygraber commented 6 months ago

Added some documentation for KMP in general (mostly generated by ChatGPT, with some minor human intervention).

eygraber commented 5 months ago

@evant anything blocking this from getting merged?

evant commented 5 months ago

Added some documentation for KMP in general (mostly generated by ChatGPT, with some minor human intervention).

I took a quick look at it and it's confusing and unhelpful, I can't accept this in this state. I would go into details but I'm not spending more time on it than you took to generate it.

eygraber commented 5 months ago

Well the generation was pretty quick, but I did spend 30ish minutes tuning it, and trying to get the tone to match the other documentation. It's a difficult problem/solution to describe, and it's even more difficult to simplify.

Because I'm more familiar with the issue I have trouble seeing when it's confusing, so I was hoping to use this a springboard for feedback on what direction to take the docs, what to focus on, clarify, etc...

eygraber commented 5 months ago

@evant I updated the documentation to be fully human written :sweat_smile:

evant commented 5 months ago

Yeah much better lol. Actually there was a start of this already with https://github.com/evant/kotlin-inject/pull/339, I'm going to make some adjustments to it and merge it in first, then we can combine it with the new annotation instructions here. I'm also working on updating the greeter sample to better align, albeit with manual expect/actual for now.

evant commented 5 months ago

https://github.com/evant/kotlin-inject-samples/pull/19

evant commented 5 months ago

Ok merged, would suggest the last section should be replaced with annotation use, also feel free to pull anything else your wrote over if you feel it would be helpful. (I think the first section mentioning the tradoffs about generating for the common source set would be good too)

eygraber commented 5 months ago

I'll work on merging my docs in; hope to have it done some time next week.

I also thought of a much better annotation name than TargetComponentAccessor, but I forgot to write it down and I forgot it :see_no_evil:

While writing the docs it didn't feel like TargetComponentAccessor is really a great name. It might be technically correct, but I don't think it communicates its purpose to users. I'm going to spend some time trying to remember the better name that I though of.

Another thing I thought of was keeping it in a separate kmp runtime artifact, so that users not using kmp don't accidentally use it (it might also help with naming the annotation because it can be more on the nose about being for multiplatform). What do you think?

evant commented 5 months ago

While writing the docs it didn't feel like TargetComponentAccessor is really a great name

Yep, absolutely agree, I'll let you know if I think of anything too. After updating the sample project I do feel like I have a bit more handle on the problem than I did before.

Another thing I thought of was keeping it in a separate kmp runtime artifact

Hm, yeah it's a hard to know long-term which would be the right decision there, but that's something we can always change later if we need to so I say go for it.

eygraber commented 5 months ago

Pushed a bunch of updates. I made some changes to the existing docs. I'll annotate my reasoning there, but I'm fine to revert any of it if you feel that it shouldn't be changed.

eygraber commented 5 months ago

Addressed feedback, and waiting for decision on @KmpComponentCreator vs @CreateKmpComponent

evant commented 5 months ago

Let's go with CreateKmpComponent then

eygraber commented 5 months ago

Done!

eygraber commented 5 months ago

I thought about it a bit more and CreateKmpComponent is technically a misnomer because the Component isn't "KMP".

Maybe CreateComponentForKmp would be more accurate?

evant commented 5 months ago

Hm, yeah. I'm starting to second-guess my suggestion too, we are really focusing on the create here and not the Component. How about @KmpComponentCreate? I know it's basically the same as what you originally suggested heh.

eygraber commented 5 months ago

Haha that works for me. I'll push that in a couple of minutes.