[Discussion] Switch from Field Injection to Constructor Injection

[oniatus]

I would like to discuss a change in the dependency injection, aka how @In works.

Current Solution At the moment we use field base injection, means we annotate fields like so:

@In
private WorldProvider worldProvider;

This works in ComponentSystems and some other classes. The main mechanism is to create an instance of the class by reflection, then later on initialize the fields using the InjectionHelper which pulls the required instances from the core registry/the context.

Proposed Solution Instead of doing so, I suggest to use constructor injection instead. Like so:

@In
public MySystem(WorldProvicer worldProvider){
    this.worldProvider = worldProvider;
}

A class for dependency injection has to provide exact one constructor, annotated with @In. All fields must be available during injection, otherwhise the injection will fail. We have an existing implementation for default constructor injection in InjectionHelper which does a bit more (like falling back to constructors with less parameters if not all fields are available), this one is used for CommandParameterSuggester and TypeHandler. I would not like to use this system as it is less intuitive. Instead we can check in a second step if we can migrate these two types to the constructor injection with @In and throw the default constructor injection away.

Should we change, we can support the field injection for a year or so and print a warning with instructions how to switch to constructor injection (straightforward change). Then I would like to remove field injection support at all in the next major version bump.

Reason

1. Field Injection is short but it helps to hurt the design without feeling the pain. Example: LocalPlayerSystem starts like this:

   @In
   NetworkSystem networkSystem;
   @In
   private LocalPlayer localPlayer;
   @In
   private WorldProvider worldProvider;
   private Camera playerCamera;
   @In
   private MovementDebugCommands movementDebugCommands;
   @In
   private PhysicsEngine physics;
   @In
   private DelayManager delayManager;
   
   @In
   private Config config;
   @In
   private InputSystem inputSystem;

   Without reading further, I can tell that this class does way too much, violates the single responsibility principle and needs some real love ;) The main problem is, that you could easily add more of such fields without any problems. However, assume a constructor like this:

   @In
   public LocalPlayerSystem(NetworkSystem networkSystem, LocalPlayer localPlayer, WorldProvider worldProvider, MovementDebugCommands movementDebugCommands, PhysicsEngine physics,
           DelayManager delayManager, Config config, InputSystem inputSystem) {
       this.networkSystem = networkSystem;
       this.localPlayer = localPlayer;
       this.worldProvider = worldProvider;
       this.movementDebugCommands = movementDebugCommands;
       this.physics = physics;
       this.delayManager = delayManager;
       this.config = config;
       this.inputSystem = inputSystem;
   }

   My first reaction would be something like "wow, that's some ugly piece of code", would you like to add more parameters to it or would you like to remove some others first to make it easier to work with? And no, it's not the representation which makes such a class that ugly, it's the number of dependencies ;)

2. The class is not testable without the dependency injection framework. To put a class with field injection under test you would need either a constructor with all the fields (something you get for free if you use constructor injection) or build it via the InjectionHelper. The main problem is, if someone adds a new field your test may compile and maybe stay green but you miss that you should update the tests. Constructor injection will break your test class on compile level because a new parameter is missing and actual force you to look at the tests.

3. It is not clear when fields are initialized. We had some issues in modules when people tried to access injected fields in the constructor. This does not work with field injection because the constructor is called bevore fields are injected. This is documented in the API but still counterintuitive to people new to dependency injection land. Constructor injection on the other hand is intuitive and allows direct access to the fields.

Some additional points:

• I am not sure if we have issues with circular dependencies but thats another design problem to solve and nothing I would support with the injection framework.
• We may provide setter injection for optional dependencies or an optional annotation if needed.

[kaen]

I agree that it would help achieve your goal of making overworked classes more obvious. Personally I'm not sure the juice is worth the squeeze. Even given a full year, this still requires every module that currently exists to be updated (although I'll grant the modification is straightforward and might even be automated).

I'm not sure the problem of people overworking their classes can be solved ergonomically, though I appreciate the cleverness of this proposed solution.

[emanuele3d]

I had a discussion with @immortius some time ago over injection. The conclusion of it was that ideally we avoid/deprecate @In-based injection and, as much as possible, pass arguments to constructors, falling back on obtaining things from a Context if really necessary.

I do agree with your analyses of the symptoms though @oniatus. Can you clarify for me what the difference would be between a standard constructor and one annotated with @In? I'm not particularly fond nor knowledgeable of annotations and don't quite fully understand what you are proposing.

@kaen is right though: the @In annotation is used everywhere in the codebase. We might somehow communicate to the crew a new way of doing things so that -when- something is touched for other reasons we also update it to match current coding standards. I'm not sure I'd have the ambition to change everything in one go.

[oniatus]

@kaen I don't think people will overwork classes automatically, that requires a lot of knowledge about refactoring for the large classes and a good understanding of design principles for the smaller or new ones. But at least it makes the pain more sensible and hopefully someone reaches out for help or an advise how to do it better.

@emanuele3d I am not determined about @In for the constructor, my line of thought was to give a clear expression which constructor is used for dependency injection. We can also move the code to a new version step by step in multiple small PRs by time, no need to fix all 450 annotations in the engine in one big go :) for future developers a warning on field injection with a link to a better workflow may be enough.

[Cervator]

I reeeeally like the simplicity of @In as it stands, although I'll admit to not being well-versed in the architectural impact, especially on unit testing.

I'm not sure introducing pain just to encourage authors to use fewer fields (even impacting valid cases with only one or two fields being used) is the right way to go. How about preparing a code analysis warning that'll detect when more than x @In instances are being used in a single class? And maybe similar warnings for complexity.

The architecture / unit testing angle is different and independent IMHO. If that leaves @In problematic enough that's something we should consider. Alternatively is there anything we can improve in the test scaffolding that'll allow somebody to readily test/mock/handle initialization order well enough to both support the super simplicity as well as great tests?

[syntaxi]

I agree with Cervator that the current @In is nice and simple and I'd lean towards that.

I'd also say that the issues caused by people using the constructor stems more from people not properly understanding the ECS style. To me using the constructor treats the System too much like an Object. If you want to run something when the system begins use the appropriate methods (postBegin and similar)

I think that the best way to solve the issue of overworked systems is to be more strict when reviewing PR's, that will stop more systems entering the codebase. The ones already in the codebase would still exist, but both solutions would leave that.

On another note, every single instance of injection across everything that uses the @In would need to be changed over to the new format. This is an incredibly big undertaking, even using automation.

As for testing, shouldn't someone that adds a new field update the test anyway? It's true that it would force the tests to be updated because it would simply not compile but at the moment quite a few PR's are accepted without even a mention of testing. If we wanted to force testing then we should also include errors when there is no test detected.