vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
622 stars 166 forks source link

Request for a Spring ViewScope as in V8 #3755

Closed stefanuebe closed 6 years ago

stefanuebe commented 6 years ago

We have an application with several views, that contains grids with huge amounts of data and a lot of components. Parallel to this data there are editor dialogues and sidebars, that show detail data or allows the editing of this data. Dialogues and views are communication via Springs ApplicationEventsPublisher.

The problem here is, that if we use "prototype" as a scope, the event communication does not work since he creates new bean instances for this communication.

On the other hand the "ui" scope leads to an unnecessary usage of memory when switching to other views, since the unused view will not be needed for a time (and since we do not know, if it will be needed again, we can not just remove it). Also they will be still react on events even if not necessary.

ViewScope of V8 handled this scenario perfectly (at least in theory). Since this scenario does not seem to be a rare scenario for a Spring based Vaadin application, please bring back the ViewScope.

Legioth commented 6 years ago

The reason this hasn't been implemented so far is that nobody has come up with a good definition of exactly when a "view" scope would change. This was (relatively) straightforward with the quite simple one-level Navigator architecture in e.g. Vaadin 8, but gets quite messy with the more versatile routing architecture in Flow.

Some interesting examples include

kumm commented 6 years ago

I am experiencing with view context on the CDI side, and have some plans what to implement.

Initially bound view context to view target lifecycle controlled by Vaadin navigation. When Instantiator::createRouteTarget is called with the route target of the navigation, the scope should change before creation. - Now i am learning what is the answer to your questions under this circumstances. Guess all of them is no. Seems createRouteTarget is called only if target component does not exist in the previous route component chain -

Next step to introduce view context strategies like in official vaadin CDI addon 3.0. This way it would be configurable per view, when to change scope. - Now it seems to be hard to implement, so next means maybe some time -

Of course there are many details currently i can't see, these are my inital toughts.

kumm commented 6 years ago

Updated my previous comment. Still think let the framework decide. Calling Instantiator with the route target means a scope change. Strategies are delayed.

Legioth commented 6 years ago

One challenge that came to mind is the situation during navigation when old components have not yet been detached, but new components have already been created. This has also been a problem with implementations for Vaadin 8.

The only "pure" solution would be to change Flow to fully get rid of the old components before even starting to create any new ones. Supporting this would slightly complicate some internal logic that is already quite heavy, but it might still be worthwhile.

Legioth commented 6 years ago

One case that I'm still pondering is when the same component class has @Route and is also a RouterLayout for other @Route classes.

If we were to simplify scope implementations by splitting up Instantiator.createRouteTarget into separate methods for route layouts and the actual navigation target, then I'm wondering whether we would also have to refrain from reusing an instances whose role changes? Otherwise, there might be cases when the active navigation target changes, but Flow isn't invoking the method for retrieving a (new) navigation target instance since it reuses an instance that was previously used as a router layout.

stefanuebe commented 6 years ago

Is there a meaningful use case for having a class having @Route and being the router layout at the same time? In v8 it was not possible to be a view and the view display at the same time afaik, maybe this should simply be the same in v10?

stefanuebe commented 6 years ago

Sorry, wrong button

kumm commented 6 years ago

As @stefanuebe currently I am thinking about a good reason to support a layout-and-target in one class.

@Legioth: Indeed removing old components from the tree before destroying their contexts would be great. Now seem to work with pseudo scopes (without client proxy) as in V8, but it should be avoided.

pleku commented 6 years ago

Layout-and-target is a very common thing to do, for instance when you have a master-details type of view structure, and there isn't always a details opened. So for instance you have a Grid in as the master data browser, and on selection you open the form on the side of the grid to edit the data. There I think everyone will use the view with the grid as both a target and a layout.

stefanuebe commented 6 years ago

How about adding parameters to the annotation to solve some of the problems, @Legioth mentioned in the beginning?

Lets say we have a @ViewScope (or maybe better called @RouteScope since it is controlled by @Route and not View anymore) with optional boolean arguments, that control the behavior for such special cases:

So each developer can handle those cases related to their own use case or work flow and we do not have to handle all special ones in a default implementation. The names of the parameters are of course just suggestions ;)

kumm commented 6 years ago

@pleku in this case why not create a @Route(value="", layout=MyMasterGrigLayout.class) with a label 'please select an item', and leave MyMasterGrigLayout to be just a ParentLayout?

Of course it is not a real solution in a DI environment. The master Layout have to be dependent, or UI scoped. We could handle the top level route target as the context driver Route target. In this case all child route of the master layout-and-route, and the master itself (the whole subtree) would belong to the same view scope. But since there can be endless level, i don't think one scope is enough. It sounds me something like conversation ( or @ConversationGroup in Deltaspike ) context.

@stefanuebe it is similar to view context strategies. I've delayed it because i've found it really hard to implement with the current concepts of the navigation in Flow. Currently Flow makes the decision to query, or not to query a route component. Would need to change it ( or would require to reimplement navigation inside an addon ).

Legioth commented 6 years ago

I suspect something like keepInstanceOnRouteAndLayoutSwitch would be really weird in practice. It would require that the child views could appear into the scope when navigating into the details view and then disappear again when navigating back. Alternatively all potential children would have to be included in the scope all the time. And then things would get even more weird if the same pattern is repeated for multiple levels (i.e. if the child is also a layout-and-target).

I agree that it might make more sense to somehow allow defining any component as a context root, destroying the context when that component is detached. I would assume that the target component would then be identified with a class literal in some annotation, with the requirement that there's only one component of that type attached in UI.getCurrent().

If I correctly understood the @ConversationGroup documentation from DeltaSpike, this is technically not a scope, but rather a qualifier that is used for similar purposes as a scope. Would this mean that this could work for injecting beans, but not necessarily for other features that are based on scopes, e.g. event propagation?

kumm commented 6 years ago

It was a long time ago I used it with JSF, but I don't remember any missing CDI features. The scope there is @GroupedConversationScoped, and beans are grouped by the qualifier. Event propagation should work regardless observer beans are qualified or not.

It is not a problem to implement a @RouteComponentScoped like that one. But of course there are some downside of this concept.

It is easy to add a detach listener destroying the group of the route component. But can't guarantee this will be the last listener. It is questionable when should the detach happen. Issues like detach after context close would not encounter even with the current behaviour.

denis-anisimov commented 6 years ago

Issue moved to vaadin/spring #263 via ZenHub