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

Allow @Route on method #2666

Open rpofuk opened 7 years ago

rpofuk commented 7 years ago

When i create new @Route("") i want to be able to add annotation to method which returns div in order to avoid extending Div class. So vaadin should support case when @Route is on class and method level.

i.e.

@Route("some/path")
public class SomePathComponent extends Div {
  public SomePathComponent() {
    setText("Hello @Route!");
 }
}
public class SomePath {
  @Route("some/path")
  public Div navigateTo() {
       return new SomePathComponent();
  }
}
pleku commented 7 years ago

Hello. Could you please clarify how using the method would provide value here in contrast to the class approach ? Just for clarity, the only requirement for the @Route class is that it is a Component, it does not have to be a Div.

I'm trying to understand what would be the added value from there, so you could mayhaps extend your example a bit. I'm guessing one advantage might be to be able to the same component for different paths like

public ProfileRoute extends BaseRoute { // can be another class than the one returned by the methods ofcourse
  @Route("profile")
  public Component openProfile() {
      return this;
  }

  @Route("profile-admin)
  public Component openAdmin() {    
    // do additional stuff
    return this;
  }
}

Then again, I'm not sure why would we need to have those as instance methods that return something, but rather just methods that are triggered for an object of that type.

Anyway, I'm not sure if we want to make this approach possible, since it would compete with the annotation-per-class approach and allow confusion for apps which would mix both ways of doing things. Where would you put the HasUrlParameter interface definition in the method case, to the component class being used ?

rpofuk commented 7 years ago

I created similar issue in Vaadin 7 https://github.com/vaadin/framework/issues/7944

1) Advantage is that I have no state in my view class 2) We usually prefer composition instead of hierarchies, they are hard to refactor

What is BaseRoute in your example?

Builders would be perfect solution for building components. Like this: https://vaadin.com/directory/component/fluent-api-for-vaadin

Not having to extend anything for my paths makes builder pattern easier to use.

You can always support both options :)

Legioth commented 7 years ago

Using methods as navigation targets would break one fundamental aspect of the current design: The ability to use a class literal for generating a URL to navigate to.

Right now you can do new RouterLink("Some", SomePathComponent.class), and the framework will take care of finding the right URL to link to. How would that work with annotated methods?

If you don't want to directly extend e.g. Div or VerticalLayout, you can also declare your navigation target as a Composite.

rpofuk commented 7 years ago

Simply saying that "routable" class hast to have class level annotation and extend Componen or one and only one method which is annotated with @Route and returns component. This way Single resposibility of class is still satisfied and rotatable classes are composable.

@Route("some/path")
public class SomePathComponent extends Div {
  public SomePathComponent() {
    setText("Hello @Route!");
 }
}
public class SomePath {
  @Route("some/path")
  public Div navigateTo() {
       return new SomePathComponent();
  }
}

Its more about composing things and avoiding extending. So different base class would still break this pattern.

rpofuk commented 7 years ago

There is also ticket regarding fluent api: https://github.com/vaadin/flow/issues/486 Which would also benefit from this concept.

Legioth commented 7 years ago

Direct routing would work if only one method on the class is allowed to have @Route.

What about other aspects of routing, e.g. stateful methods such as HasUrlParameter::setParameter and HasDynamicTitle::getPageTitle?

rpofuk commented 7 years ago

Something like this would be acceptable:

    public class SomePath implements HasDynamicTitle, HasUrlParameter<Long> {

        private Long parameter;

        @Override
        public String getPageTitle() {
            return BackendService.instence().getTitle(parameter);
        }

        @Override
        public void setParameter(BeforeNavigationEvent event, @OptionalParameter Long parameter) {
           this.parameter = parameter;
        }

        @Route("some/path")
        public Component navigateTo(BeforeNavigationEvent event) {
           return SomePathComponent.builder()
                       // DO MY FENCY BUILDING
                       .build();
        }
    }
Legioth commented 7 years ago

How does navigateTo take the URL parameter into account? Would it be necessary to call navigateTo every time the parameter changes? Would that mean that a new component instance is created when only the parameter changes but the user still stays on the same view?

rpofuk commented 7 years ago

Yes. of course. View should be immutable once its rendered. Its less error prone.
Sacrificing immutability to save little bit of performance (if any) is not worth it.

And i think that manipulation of parameters to remain on same view is edge case does not justify mutability and state.

Legioth commented 7 years ago

On the server, the performance impact is indeed quite small since we only recreate some objects and sync them to the client. The problem is on the client where lots of things would have to be reinitialized, in many cases causing things to visually flicker on the screen.

rpofuk commented 7 years ago

I could always save current view in private variable and setParameter to modify that view to cover case you mentioned. So SomePath class in my example can be reused, if it stateless componets will be recreated and if I want to have fresh component every time i would keep it stateless.

ctrohin commented 6 years ago

Routing annotations on methods would allow everyone to separate functionality related to a given object or group of objects, in a single class. Much line the Controller concept in standard MVCs. It's pretty cumbersome to implement one class to show object X, one class to edit object X and one to display a list of objects from same class as object X. The alternative, is to use parameters, or the context path, but the routing concept would then be defeated.

Legioth commented 6 years ago

@ctrohin I'm not sure I understand what you mean. In what way is a class more or less cumbersome than a method?

MVC is typically a stateless pattern, whereas the component tree shown for a specific route in Flow is stateful. Editing, viewing and listing entities will typically use different component tree structures. From this follows that each case would have a different class with unique construction logic and a different set of instance fields for the state.

While the state would be different for each case, it's still possible to share logic through a common super class or a util class. Static inner classes can also be used to help keep all the UI logic related to one entity type in the same Java file.