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

[plugins] Introduce strict setup with abstract classes #2006

Merged
merged 4 commits into from Oct 28, 2023
Merged

Conversation

tipsy
Copy link
Member

@tipsy tipsy commented Oct 1, 2023

No description provided.

@tipsy tipsy mentioned this pull request Oct 1, 2023
3 tasks
@tipsy tipsy force-pushed the plugin-revisit branch 3 times, most recently from ccec4d1 to 75e95b6 Compare October 26, 2023 04:24
@tipsy tipsy changed the title abstract plugins [plugins] Introduce strict setup with abstract classes Oct 26, 2023
@zugazagoitia zugazagoitia mentioned this pull request Oct 27, 2023
7 tasks
javalin/src/main/java/io/javalin/config/JavalinConfig.kt Outdated Show resolved Hide resolved

fun <CFG : PluginConfiguration> Consumer<CFG>.createUserConfig(cfg: CFG): CFG =
cfg.also { accept(it) }
abstract class NoConfigPlugin : JavalinPlugin
Copy link
Member

@dzikoysk dzikoysk Oct 27, 2023

Choose a reason for hiding this comment

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

I don't like the fact that JavalinPlugin interface is implemented by some abstract Plugin and abstract NoConfigPlugin.

  1. If we want to have both interface & abstract class, it should be called like AbstractJavalinPlugin/StandardJavalinPlugin/etc.
  2. The fact that there's JavalinPlugin interface already removes the need to follow our requirements defined by the abstract constructor (and that was one of the main reasons to start this change 🤔)
  3. Instead of making a 3rd structure that represents a no-config plugin, you could just make the config parameters optional, so Plugin<Void> already tells us that there's no cfg.

I'd personally just stay with one abstract JavalinPlugin class, instead of making variants of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Why should it be called that?
  2. In this PR, registerPlugin accepts either a Plugin or a NoConfigPlugin, the JavalinPlugin interface is just used internally. The registerPlugin function does not accept JavalinPlugin.
  3. That's how I had it, but it becomes very hacky. You need to cast a null to be a non-null (or have a nullable config), and you get a class that has a pluginConfig property that sometimes doesn't actually exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

One downside here is that people could potentially create their own NoConfigPlugin with a custom configuration 😓

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the hack is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an exception to make it less hacky.

@tipsy tipsy merged commit 69167aa into master Oct 28, 2023
18 checks passed
@tipsy tipsy deleted the plugin-revisit branch October 28, 2023 11:56
@zugazagoitia zugazagoitia mentioned this pull request Oct 31, 2023
14 tasks
@rob-bygrave
Copy link

abstract class Plugin<CONFIG>

Just to say that from the perspective of avaje-http code generation for Javalin, the Plugin being an interface was nice. The change to have this instead as an abstract generic type seems to my eyes unexpected and a touch ugly. Maybe I don't understand the design yet or it is specific to how this is used by avaje-http (code generation of routes).

To me, it almost looks like there are 2 concepts in play there now with the generic type / consuming the generic type.

For the avaje-http use, it is generating a "Route" and with Javalin 6 beta this is something that now extends Plugin<Void> and to me that seems less nice relative to implementing an interface (which seemed more natural for the avaje-http code generation).

@dzikoysk
Copy link
Member

dzikoysk commented Nov 1, 2023

Indeed, it is specific to what avaje-http seems to be doing. Javalin's API is focused on our end-users (so probably like 99%+ of people that don't write plugins, but use these already available in the ecosystem). Before 6.x, each plugin was kinda a new independent world, because plugin authors defined the API & init phase the way they wanted, not the way that matched some kind of official guideline. That's something we've decided to address, so since 6.x, users should expect the same approach across different plugins:

Javalin.createAndStart { cfg ->
  cfg.registerPlugin(SslPlugin {
     it.abc = true
  })
  cfg.registerPlugin(OpenApiPlugin {
     it.def = false
  })
  cfg.registerPlugin(AbcPlugin()) // plugin without config (repredented by Plugin<Void>)
}

Plugins are represented by the abstract class that requires a supplier & default cfg in the constructor, so it creates a natural requirement from authors to follow that pattern.

Despite the fact, that having something like AvajeHttpPlugin extends Plugin<Void> shouldn't really be an issue, if your plugin is focused only on routing, you may just use a new routing API:

class AvajeHttpRouting : RoutingApiInitializer<Void> {
  override fun initialize(cfg: JavalinConfig, internalRouter: InternalRouter, setup: Consumer<Void>) {
    // register your routes directly in `internalRouter` or via cfg
  }
}

Javalin.createAndStart { cfg ->
    cfg.router.mount(AvajeHttpRouting())
}

@tipsy
Copy link
Member Author

tipsy commented Nov 2, 2023

@dzikoysk gave a very nice summary. Our main focus is on plugin consumers (hehe), not plugin authors. That being said, we do of course want to make things as nice as possible for plugin authors too.
Is there anything the new approach is preventing you from doing, or is it just a bit more complicated?

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

Successfully merging this pull request may close these issues.

None yet

4 participants