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

Add codegen mode to support both Anvil and Hilt #963

Merged
merged 13 commits into from
Nov 23, 2023
Merged

Add codegen mode to support both Anvil and Hilt #963

merged 13 commits into from
Nov 23, 2023

Conversation

jamiesanson
Copy link
Contributor

This PR implements a KSP argument for CircuitSymbolProcessor which allows a consumer to skip emission of Anvil annotations. One use case of this is to allow for generated binding support for other DI frameworks, such as Hilt.

The argument "circuit.generate-anvil-bindings" defaults to true, and can be configured via the KSP gradle DSL:

ksp {
  arg("circuit.generate-anvil-bindings", "false")
}

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @jamiesanson to sign the Salesforce Inc. Contributor License Agreement.

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Given there's another request for supporting kotlin-inject, let's make this a "mode" instead. Then it becomes trivial to add new types that support this kind of injection

circuit.codegen.mode=anvil where options are anvil and hilt for now. Can stick 'em in an enum that maybe holds information about the contributor annotation type and param?

enum class Mode {
  ANVIL {
    override fun annotateFactory(...) {
      ...
    }
  },
  HILT {
    override fun annotateFactory(...) {
      ...
    }
  };

  fun annotateFactory(builder: TypeSpec.Factory, scope: ClassName)
}

@jamiesanson
Copy link
Contributor Author

Sounds like a plan!

Given your suggestion on naming of the option being codegen mode, what are your thoughts on me pulling in my Hilt module generation changes too? I'd prefer to have this in circuit rather than living in my own third-party artefact, but also get it if you're not keen to take on that much scope

@ZacSweers
Copy link
Collaborator

As long as it's simple it's fine with me. Seems like it's trivial to add in hilt's case

@jamiesanson jamiesanson changed the title Add KSP argument to skip emission of Anvil annotations Add codegen mode to support both Anvil and Hilt Oct 26, 2023
@jamiesanson
Copy link
Contributor Author

Just got around to adding the Hilt codegen mode option as suggested. Tested against a seperate project with a local version and it's working as expected! Docs-wise, is there anything to contribute to?

The argument "circuit.generate-anvil-bindings" defaults to true, and can be configured via the KSP gradle DSL:

ksp {
  arg("circuit.generate-anvil-bindings", "false")
}
The argument "circuit.generate-anvil-bindings" has been renamed to "circuit.codegen.mode", with support for Hilt added by generating additional files implementing dagger modules
@jamiesanson
Copy link
Contributor Author

Apologies for the delay on this, been AFK for a while due to travelling. The recent commits I've pushed have added in documentation, tidied up errors, and implemented the remaining Hilt functionality with @GeneratesRootInput and @OriginatingElement where applicable. Let me know what you think!

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

I'd like to avoid imposing the hilt dependency on annotations, is it not possible to do that?

Comment on lines 29 to 31
api(libs.anvil.annotations)
api(libs.dagger)
api(libs.hilt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's actually remove all three of these dependencies now since we don't want to impose a specific framework anymore

Copy link

Choose a reason for hiding this comment

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

if the hilt meta-annotation is required for CircuitInject, would compileOnly() for the hilt dependency work?

I know for the JVM annotations don't actually need to be on the classpath at runtime

Copy link
Collaborator

Choose a reason for hiding this comment

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

That could work, though still not thrilled about requiring this at all. Could you or @jamiesanson explain it a bit more? I don't know that much about Hilt these days

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GeneratesRootInput annotation exists for repeatability and correctness when using Hilt, due to how it detects modules. It allows the Hilt compiler to see that other processors are going to produce input for it, meaning it can wait until other codegen steps complete before it wires up its own dagger components. Hilt won't necessary mess things up if this annotation isn't there, but it's not reliable without it. Docs for hilt extensions are here if you're interested in a quick browse: https://dagger.dev/hilt/creating-extensions.html

It's definitely not needed at runtime, and is retained at class-level, so should be fine making this compileOnly

* For more general information about this annotation, see
* [com.slack.circuit.codegen.annotations.CircuitInject]
*/
@GeneratesRootInput
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this required for hilt to work? I'm not sure I'm comfortable imposing the dependency on anyone using this artifact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, however as mentioned compileOnly should do the trick. If you're not comfortable with this constraint on dependency here, happy to discuss potentially creating a new optional module for hilt-specific annotation. That'll also introduce a nitpick-y improvement in that we can rename the scope parameter to component to tie in to Dagger vocabulary, so maybe worth doing anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'm ok with compileOnly. @stagg @kierse any concerns?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it works with compileOnly we're all good, do like the idea of keeping all the codegen together given the current overlap.

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Awesome work. There's a few followups I think we'll want to make, but don't want to keep you attending to this as we've taken awhile to review this. Very much appreciate you taking the time and the patience!

@ZacSweers ZacSweers added this pull request to the merge queue Nov 23, 2023
Merged via the queue into slackhq:main with commit f5a2bb2 Nov 23, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants