vaadin / flow

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

Enable redirecting from navigation target's constructor #3181

Open pekam opened 6 years ago

pekam commented 6 years ago

Based on Spring addon's DX tests it's very common for users to try redirecting from a navigation target's constructor, based on properties of an injected bean.

Example: Trying to open a page without being logged in -> redirect to login-page.

@Route("loggedin")
public class LoggedInView extends Div {
  public LoggedInView(@Autowired UserBean user) {
    if(user.getUserName() == null) {
      UI.getCurrent().navigateTo("login");
    }
  }

Currently this throws NPE since UI.getCurrent() returns null until the component has been attached.

You can avoid this issue by implementing onAttach() and writing the redirection logic there, but the testers didn't figure this out.

So we should make it possible to do server-side navigation from a component's constructor.

pleku commented 6 years ago

I have no idea how we could provide e.g. a bean as a parameter in the constructor. Would be quite magic or at least some new feature that is waiting to be invented. Or mayhaps the snippet is missing some part that like @Autowired User user ?

So couple things: 1) Regardless of should the UI.getCurrent() not return null when the bean is being initialized, for me it makes absolutely no sense to get/give the UI for the constructor and do any redirecting there. The bean is just being constructed at that time, would be really weird (and really cumbersome to support) any redirecting during the initialization since the navigation target will attached and navigation lifecycle events triggered at later time. And they can assign to the lifecycle and handle the redirection there.

2) The thing is that the users should be allowed to easily discover the answer to the question when they can redirect, when using Spring or something else. Mayhaps we should have an example on this, something like

@Route("home")
public class HomeView extends Div {
  @Autowired User loggedInUser;

  public void beforeNavigation(BeforeNavigationEvent event) {
    if (loggedInUser == null) {
      event.rerouteTo(LoginView.class);
    }
  }
}
pekam commented 6 years ago

Yes, the example missed Spring dependency injection. My bad, I was sloppy while writing it and didn't realize this is only possible with Spring. I edited the example and added "Spring"-label.

It has been proved by three users (including myself) that the most intuitive thing to do in the above case is to do null-check and redirection in the constructor. Users who still haven't dived into the routing documentation expect that the view is constructed when you enter there and that's it. All those lifecycle events seem redundant in many cases and will cause extra thinking for the users, as things didn't work out as they expected.

But since this would work only with Spring anyway, I don't care that much... Just noting that we should put great value on intuition, as it provides the best DX when you just do what feels natural and everything works.

heruan commented 6 years ago

In my experience with CDI and Spring I'd never assume to have thread-related beans in the constructor, such as the UI or the authenticated user, since AFAIK the component instance may be constructed before those value are available and thus I'll trust more the lifecycle methods, or in the constructor I'd postpone my logic with addAttachListener.

Otherwise, one would expect to also have the URL parameter for HasUrlParameter<T> available in the constructor if the instance is created for each different path.

pekam commented 6 years ago

Good to know it makes sense for experienced users. I guess that's the bigger target group anyway compared to Spring-newbies trying it out with Flow.