I'm reposting this in github https://github.com/google/guice/issues/1636, as this forums seems to miss some love.
Please, answer there. On Monday, June 13, 2022 at 5:37:00 PM UTC+2 Alberto wrote: > Dear List, > > I'm trying to debug an issue ( > https://github.com/serenity-bdd/serenity-gradle-plugin/issues/9) in a 3rd > party framework that uses Guice. The problem has to do with the integration > of the framework (serenity-bdd) in a gradle plugin, where some instance > isn't recreated when needed. > > Inspecting the code, I can summarize what I fell is a bad practice. The > code has a Singleton more or less like this (see in the sources: > https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-model/src/main/java/net/thucydides/core/guice/Injectors.java > ): > > public class Injectors { > private static Injector injectors; > public static synchronized getInjector() { > if (injector == null) { > injector = Guice.createInjector (new MyModule()); > } > return injector; > } > } > > and then uses Injectors.getInjector() to get instances of classes > everywhere through the code. For instance (see in the sources: > https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-core/src/main/java/net/serenitybdd/core/pages/PageUrls.java#L118-L124 > ): > > private static String namedUrlFrom(String annotatedBaseUrl) { > String pageName = > annotatedBaseUrl.substring(NAMED_URL_PREFIX.length()); > EnvironmentVariables environmentVariables = > Injectors.getInjector().getInstance(EnvironmentVariables.class); > return EnvironmentSpecificConfiguration.from(environmentVariables) > .getOptionalProperty(pageName) > .orElse(environmentVariables.getProperty(pageName)); > } > > There are also many places throughout the code where constructors uses the > Injector to initialize its fields. For example (see in the sources: > https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-core/src/main/java/net/serenitybdd/core/annotations/locators/SmartAnnotations.java#L165-L167 > ): > > public SmartAnnotations(Field field, MobilePlatform platform) { > this(field, platform, > WebDriverInjectors.getInjector().getInstance(CustomFindByAnnotationProviderService.class)); > } > > > I have no experience using Guice, and no experience with the framework, so > I cannot say if it's very wrong or acceptable. But this (anti?)pattern is > giving me hard times trying to debug. As it is now, many functions are > quite compact, but I guess we are loosing many benefits of Guice, and I'm > not sure if this is a legal use at all. > > I have began to rewrite the code and prepare a Pull Request, but it's > extremely costly and I'm not sure that I will do better than the current > code. So the question for the experts: Is this so bad that it would require > a complete rewrite of the relevant code? Am I missing something? > > Best regards, > > Alberto > -- You received this message because you are subscribed to the Google Groups "google-guice" group. To unsubscribe from this group and stop receiving emails from it, send an email to google-guice+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/38c4e410-9d4f-407b-b836-9c613b18ae8fn%40googlegroups.com.
