Deprecate @InjectViewModel and @InjectScope and move to JSR330 (@Inject)

[sialcasa]

@lestard please argue why this is not possible

[sialcasa]

An additional argument: If you accidentally inject your scope with @Inject instead of @InjectScope, you'll get the wrong instance. The same happens with the other annotations. There is a high risk of usage mistakes by the user.

[manuel-mauky]

There are technical reasons on why we haven't used JSR330 in the first place: If you use CDI we have the following livecycle when a ViewModel is injected into a View:

1. ViewLoader requests a View instance from CDI
2. CDI creates an Instance of the View class
3. CDI injects all dependencies marked with @Inject into the View instance
4. CDI calls @PostConstruct on View class
5. ViewLoader gets the Instance of the View
6. ViewLoader requests a ViewModel instance from CDI
7. Steps 2, 3 and 4 are going on for the ViewModel instance
8. Now the ViewLoader injects the ResourceBundle into the View and ViewModel instances via @InjectResourceBundle
9. Inject Scopes into the ViewModel
10. injects the ViewModel into the View via @InjectViewModel

If we would use @Inject for ResourceBundle, Scope and ViewModel, CDI would try to inject them on its own. We wouldn't have any chance to make our checks and control the process (see https://github.com/sialcasa/mvvmFX/wiki/Dependency-Injection#injecting-viewmodels-with-injectviewmodel-vs-inject).

Same is true for Guice.

[sialcasa]

Couldn't we avoid the problem using producer methods

[mainrs]

This topic hasn't been touched by a year now. I did some research and testing with some bare bones code. I couldn't get Guice to work with the @Inject annotation. It will always fail under some circumstances. Are there other frameworks that allow more fine tuning tha guice or EasyDI?

[manuel-mauky]

I think the problem isn't the DI frameworks but instead the FXMLLoader (like described above). I don't see any way of supporting this feature with the existing FXMLLoader regardless of which DI framework is used.

I'm mentoring a student at the moment who is working on an extended FXMLLoader that maybe could provide better options of integration but his work has other priorities (especially the Scopes feature) and not this issue we are discussing here. So I'm still doubtful if there will ever by a real solution.

In my opinion this issue doesn't have a high priority either because I think it's not that much of a problem to have other annotations then @Inject.

You are right: This issue wasn't touched a long time and maybe we should have closed this issue long time ago. The intention of keeping it open was to keep the general idea alive and maybe come up with another good idea of how this could be implemented.

[mainrs]

Sounds interesting, is he recreating the FXMLLoader and implementing his own parsing process? In which way is the FXMLLoader limiting the usage of @Inject? I am quite slow on the uptake, sorry.

[manuel-mauky]

I'm sorry, I realized that I've mixed some thing up in my mind. The FXMLLoader is a serious problem for the scopes feature but not in this case. Forget what I've said in the previous comment ;-)

It could be possible to provide a fully injected codeBehind class (with some limitations on the Scopes) to the FXMLLoader.

The problem is that we need to tell the DI framework at loading time what instances to use during injection. I think it the ViewModel should be the easiest. The DI framework could create a new one for each CodeBehind (with 1 exception, see below). The ResourceBundle is more problematic because you can pass a resourceBundle to the FluentViewLoader and you can define one at the fx:include tag in the FXML file. With mvvmFX these bundles get merged with a global resourceBundle which is easier to be done after the loading of the FXML file.

The exception for the ViewModel is the root CodeBehind. It's possible to pass an existing ViewModel instance to the FluentViewLoader which is used instead of creating a new one. We would need to tell the DI framework that in this special case it shouldn't create a new instance.

For CDI you can create "producer methods" which can be used to provide instances for specific classes to the DI framework so it doesn't create them by itself. We use this in our mvvmfx-cdi-module so that the user can inject the NotificationCenter and other things. I don't know if this mechanism could be used for this feature here. And I don't know any feature of guice that could help (I'm not as familiar with guice as I am with CDI).

Regarding the FXMLLoader: He has only started some days ago so it's to early to say how it will be implemented. It's not decided yet. Theres some theoretical work to be done before starting the implementation.

[mainrs]

For CDI you can create "producer methods" which can be used to provide instances for specific classes to the DI framework so it doesn't create them by itself.

Do they also allow some kind of type request? Lets say my application needs to ViewModels, if I understood the Producer Methods right, we would need one method for each ViewModel, explicitly declaring the returned type of the ViewModel. Is some generic approach as the following possible? @Produces public ViewModel createViewModel(Class<? extends ViewModel> neededClass) {}

It might be a better idea to wait till Java 9 arrives for the custom FXMLLoader. It adds some APIs that will make it more reliable (like StackWalker), allowing one to walk the visit the caller stack easily and replace Oracle's internal Reflections.getCallerClass method.

[manuel-mauky]

In a producer method you get informations about the place where the dependency is needed. The actual type would also be present. However, I don't know if CDI can use a producer method that returns ViewModel when it needs an instance for MySpecialViewModel. I assume that it would say that no class/producer for the dependency was found. I need to test this.

Thanks for the hint for Java 9.

[manuel-mauky]

I've tested it: CDI can inject MySpecialViewModel without any producer method because it finds the class and creates an instance of it.

If I add a producer that returns ViewModel then it will be ignored by CDI because it already knows how to instantiate the actual MySpecialViewModel via it's constructor.

If I add a producer that returns MySpecialViewModel then CDI complains that it now can't decide which to use. I would need to deactivate the constructor of the actual ViewModel class which is possible if you have access to the code. This is problematic because the user would need to add a CDI annotation (@Alternative) to his ViewModel class.

[mainrs]

So basically it would be possible to drop @InjectViewModel for @Inject but would remove the benefit of doing some pre-injection operations like generic type insurance, am I right?

[manuel-mauky]

We would need to drop the feature to provide an existing ViewModel instance to the FluentViewLoader. Maybe other features will break too. We would have no access to the ViewModel instance before it is injected into the CodeBehind.

[manuel-mauky]

It would also mean to be more depenend on the actual DI frameworks. At the moment we only say "DI framwork, give us an instance of Class X, we do the rest". After that we would need to create logic for initialization for each DI framework.

[mainrs]

Doesn't seem to be worth it. Is there another way we could provide a custom instance. For example, technically, we could let DI inject a ViewModel and overwrite afterwards with the one provided through the ViewLoader. It is quite ugly though and adds a lot of heap for the operation.

[manuel-mauky]

The problem with this approach (besides being ugly) would be that the user assumes that he can use @PostContruct annotation on an initializer method to work with the injected ViewModel. This method will be invoked before we have access to the codeBehind instance and therefore before we can exchange the ViewModel instance. The setup code from the user would be useless without an easy way to find out what's happening.

The second problem is that a new ViewModel instance would be created by the DI framework even though the user provides his own existing instance. This can be really confusing and sometimes even problematic when heavy logic is done durring the creation of the ViewModel, for instance, loading data from a server.

[mainrs]

The second problem is that a new ViewModel instance would be created by the DI framework even though the user provides his own existing instance. This can be really confusing and sometimes even problematic when heavy logic is done durring the creation of the ViewModel, for instance, loading data from a server.

This is my main concern. Adding another layer of lifecycle (like a second method triggered when we are done with the setup) may work but so much work for changing an annotation is, in my opinion, not worth it. I thought that there might be some tricks possible using DI or CDI...

[manuel-mauky]

It's my opinion too. This isn't worth the effort. There might be some tricks possible but I haven't had a good idea so far. But for this reason we could keep this issue open so that maybe someone finds a good idea in the future. I don't know if this is a good approach. What do you think? Independently, I will add appropriate tags to the issue to express that we are not actively working on this issue at the moment.

[mainrs]

No harms done by leaving it open. I have an idea but I need to test it first :D

[manuel-mauky]

Ok then I'm curious to see your idea when it's finished.

Just some aspects to keep in mind if we find a working solution:

• The existing solution with the old annotations still has to work. We need to support both variants in parallel for at least for 1-2 versions so that users can softly migrate.
• In the best case it should work for all DI frameworks we are supporting at the moment (CDI, Guice, EasyDI) and it should be possible to integrate new DI frameworks in the future. If we find a solution that only works with one of the DI Frameworks, we need to keep the existing Annotations without deprecating them. However, the new approach could still be supportet as optional feature. But we have to make sure that it doesn't collide with the existing approach.
• If your approach doesn't work in parallel with the existing one we could still integrate it but only in a big major release with version 2.0. At the moment we have no definite plans for such a major update but we are gathering ideas of breaking changes with the milestone 2.0. This means it could take quite some time until this feature would be released.