Closed vlukashov closed 4 years ago
Would this actually be the same as HasUrlParameterPair<T, U>
when using @Route("orders")
and HasUrlParameterPair<Integer, String>
This functionality was left out from the design in order to keep it simple.
The simplest workaround is probably to change the URL structure to /orders/edit/:id
instead.
The suggested workaround of using HasUrlParameterPair<Integer, String>
has some limitations when there are multiple different "sub routes", e.g. /orders/:id/edit
and /orders/:id/details
.
Some ideas for this could be supported:
1) Allow a router layout (or section) to have URL parameters (with some restrictions, e.g. no @Optional
or @Wildcard
).
2) Reuse the idea of using HasUrlParameterPair<Integer, String>
, but add some syntax for pattern matching on string parameters to resolve ambiguity between multiple navigation targets with the same base @Route
.
3) Add something like @RoutePrefix("edit")
that is used to resolve between multiple navigation targets with the same @Route
.
4) Support arbitrary patterns with placeholders for routes (e.g. @Route("orders/:id/edit")
). With this scheme, there's the possibility of having a mismatch between the number of placeholders in the URL and the arity supported by the navigation target class itself.
Regardless of the approach, one additional problem is that only one of the router layout and the navigation target will receive the id parameter in a typesafe way. If the other party also needs to know the id, it would either have to manually extract and convert it or the classes would have to explicitly pass the received parameter to the other instance.
On this regard, I find very intuitive and productive the JAX-RS approach, i.e.
@Path("orders/{id}/edit")
public void editOrder(@PathParam("id") Long id) {
// ...
}
@Path("orders/{id: [0-9]+}/edit")
;it can be placed on both classes and methods, to create hierarchies easily:
@Path("orders")
public class OrderRoute {
@Path("{id}")
public Component orderCard(@PathParam("id") Long id) {
// ...
}
@Path("{id}/edit")
public Component orderForm(@PathParam("id") Long id) {
// ...
}
}
BTW I'm not sure this approach, which fits well for stateless APIs, would fit just as well in Flow.
If we are going to support a placeholder syntax in @Route
values, then it seems like the URI Template syntax would be a good fit.
The overall problem with any such approach is that it's based on boilerplate annotations with magic strings (e.g. @PathParam("id")
) for mapping between URI parameters and method parameters. Those annotations could be avoided in most cases with a "magic" convention that falls back to the order in which the parameters are defined.
It might still be that that boilerplate annotations or a magic convention is the price that must be paid for supporting this level of flexibility. It would still not be needed in simpler cases where the parameters are at the end of the URL.
There is a separate discussion about allowing @Route
on methods in https://github.com/vaadin/flow/issues/2666.
Moving here some thoughts from #3474:
@heruan said:
From my perspective, JAX-RS magic is cool but also hardly maintainable without IDE plugins; e.g. Eclipse warns you if you're missing a
@PathParam("id")
argument from your@Path("/users/{id}")
method. It makes refactoring safe-ish, but without IDE help large projects with this type of syntax become quickly hard to maintain (after years of JAX-RS, we have switched all our APIs to GraphQL with a huge gain on maintainability and type-safety).That's why I would avoid string magic as hell, but still I do need to nest routes under parameterized ones and my use cases would all have also nested layouts, e.g. in the
UserCard
a top section with user main info and a lower tab-like layout with nested routes for each tab.So I see three ways out of this:
- refactoring the routing declarative approach with magic strings like
users/{id}/profile
;- leave routing as is and make nested layouts work with parameterized paths;
- do not support nesting at all.
Predictably I'd vote for 2 😄
@mvysny said:
I personally don't like magic, that's why I tend to use JAX-RS cautiously. But I have yet to see a simple type-safe alternative to JAX-RS. JAX-RS is not one of the nicest ones but it works. In the worst case of parameter misconfiguration the compiler won't unfortunately complain, but JAX-RS will immediately fail in runtime with a helpful message, so it's better than nothing.
Don't get me wrong: I am all in for compiler-enforced type safety; even better if the type system can guide you to use the API correctly. However, if this conflicts with simplicity, I tend to prefer simplicity. Of course others may choose otherwise - Scala guys, having experience by being burned by string magic, etc. There are multiple ways to skin a cat, and it depends on the target user group which should be chosen.
I love simplicity and therefore I selfishly vote for 1 :-)
@heruan said:
So your desiderata aren't magic strings, but being able to nest parameterized routes without layout requirements. Then I guess a
@ParentRouteTarget
annotation would be sufficient, e.g.@Route("departments") public class Department extends VerticalLayout implements HasUrlParameter<Long> {} @Route("employees") @ParentRouteTarget(Department.class) public class Employee extends VerticalLayout implements HasUrlParameter<Long> {} @Route("profile") @ParentRouteTarget(Employee.class) public class Profile extends VerticalLayout {}
Then the router should quite easily find
Profile
fordepartments/123/employees/456/profile
and provide all the parameters in aMap
, e.g.Long department = params.get(Department.class)
.
One of the apps I'm porting (from Aurelia) to Flow is growing fast and this would be really necessary for a smooth port. Could the scenario I described in my last comment above be approachable and fit all the use cases?
@heruan I'm not sure I understand exactly how the different parts would work together in your example. I assume that e.g. the Profile
class would need to know which employee it should show, but how would it find that information?
My example is a bit hermetic, I get it 😃 Instead of (or in addition to) a @ParentRouteTarget
annotation we could have a HasParentRoute<T, C extends HasUrlParameter<C>>
and use it like this:
@Route("departments")
public class Department extends VerticalLayout implements HasUrlParameter<Long> {
// override setParameter
}
@Route("employees")
public class Employee extends VerticalLayout implements
HasUrlParameter<Long>, HasParentRoute<Long, Department> {
// override setParameter
@Override
public void setParentRouteParameter(Long departmentId) {
}
}
@Route("profile")
public class Profile extends VerticalLayout implements HasParentRoute<Long, Employee> {
// override setParameter
@Override
public void setParentRouteParameter(Long employeeId) {
}
}
Or, for more complex case (e.g. one need a parameter deeper in the hierarchy) provide a map to obtain the parameter from the component class, e.g. params.get(Employee.class)
.
The more use cases I'm facing while porting a structured app to Flow, the more it seems to me this could be combined to router layouts without refactoring the entire routing system, staying simple and straightforward for both developers and users.
The culprit would be supporting HasUrlParameter<T>
on parent layouts, and a HasParentLayout<L extends RouterLayout>
with a method to provide the parent layout instance.
For example, say I have a RouterLayout
which provides a navigation bar with router links, e.g.
@RoutePrefix("users")
public class UserSectionLayout implements RouterLayout, HasUrlParameter<Long> {
^^^^^^^^^^^^^^^
HorizontalLaytour navbar = new HorizontalLayout();
VerticalLayout content = new VerticalLayout();
Long id;
public UserSectionLayout() {
this.add(navbar, content);
}
public Long getUserId() {
return this.id;
}
@Override
public void setParameter(BeforeEvent event, Long id) {
this.id = id;
this.navbar.add(new RouterLink(UserProfileRoute.class, id));
this.navbar.add(new RouterLink(UserOtherRoute.class, id));
}
@Override
public void showRouterLayoutContent(HasElement content) {
this.content.getElement().appendChild(content.getElement());
}
}
@Route(value = "profile", layout = UserSectionLayout.class)
public class UserProfileCard extends VerticalLayout implements HasParentLayout<UserSectionLayout> {
@Override
public void setParentLayout(UserSectionLayout parentLayout) {
Long id = parentLayout.getUserId();
User user = UserBackend.getUser(id);
}
}
@Route(value = "other", layout = UserSectionLayout.class)
public class UserOtherCard extends VerticalLayout implements HasParentLayout<UserSectionLayout> { /* ... */ }
This would provide URLs like users/123/profile
and users/123/other
and the router should be able to register the routes quite easily since UserOtherCard
is mapped to other
and has a parent layout with users
prefix and implements HasUrlParameter<Long>
.
Plus, any component in the activation chain can get an instance of its parent layout and access any parameter in the hierarchy.
Note: having parent layouts with URL parameters is the only way I can guess to add navigation links on them when their child components are parameterized too.
It seems like both alternatives (HasParentLayout
or HasParentRoute
) could make routing quite straightforward, but I'm not sure about URL generation. What would the typesafe API look like for generating a URL like users/123/profile
?
There could in theory be a monster like public <T, C HasParentLayout<? extends HasUrlParameter<T>>> String getUrl(Class<? extends C> navigationTarget, T parameter)
. This pattern would lead to a permutational explosion if we also want to support one parameter for the parent and one parameter for the child (departments/123/employees/456
) and/or multiple parameters for the same class (HasUrlParameterPair
).
One potential solution to this could be some kind of fluid builder that would allow defining one set of parameters per method call instead of doing everything through only one getUrl
invocation. It could then be something like buildUrl(parentClass, param1, param2).withChild(childClass, param3).build();
.
One additional observation is that it's seems redundant to define layout = UserSectionLayout.class
in the @Route
annotation since the same information is already in the type parameter. On the other hand, defining the parent in the annotation is more discoverable and feels slightly less verbose. Supporting both is of course also an option, even though there might also be some confusion if there are two quite similar ways of doing basically the same thing.
We might still also want to support placeholders in the route mapping strings, e.g. for cases when a view needs to access parameters from multiple parent layouts.
One potential solution to this could be some kind of fluid builder […]
Totally agree, a builder would be versatile and safe.
We might still also want to support placeholders in the route mapping strings, e.g. for cases when a view needs to access parameters from multiple parent layouts.
Not necessary if layouts provide getters, since you can descend in the hierarchy, e.g. for
@RoutePrefix("departments")
public class DeparmentsLayout implements HasUrlParameter<Long> {
// ...
public Long getDepartmentId() { /* ... */ }
}
@RoutePrefix("employees")
public class EmployeeLayout implements HasUrlParameter<Long>, HasParentLayout<DepartmentsLayout> {
// ...
public DepartmentsLayout getDepartmentsLayout() { ... }
}
@Route("profile")
public class EmployeeProfile implements HasParentLayout<EmployeeLayout> {
@Override
public void setParentLayout(EmployeeLayout parentLayout) {
Long employeeId = parentLayout.getEmployeeId();
DepartmentsLayout dl = parentLayout.getDepartmentsLayout();
Long departmentId = dl.getDepartmentId();
}
}
I'd avoid placeholders in route mappings, which in my opinion would open a Pandora's box of convoluted logic to provide the values to the user in a typesafe way (if possible at all).
Is it really that important to have URLs generated in a type-safe way? Are we willing to introduce all those builders and type safety and make users to use that, to avoid simpler but dynamic solution with placeholders? As if what happened to Scala was not scary enough.
Everyone has their own preference when it comes to potential tradeoffs with type safety, so I think we should consider supporting both approaches.
This would be in line with e.g. Binder
where you can choose between typesafe lambdas for the property getter and setter, or alternatively giving the property name as a String
and then let reflection take care of the rest.
A bit as an exercise (and much because we really need this to port our apps to Flow 😃) I've implemented a working-ish solution with router layouts: https://github.com/vaadin/flow/compare/master...heruan:layout-url-parameters
Basically, when a RouterLayout
implements HasUrlParameter
, a pattern is appended to the route prefix of that layout and the resolver matches the pattern with the requested path, collects the parameters and sets them on the corresponding instance.
Then, any component in the chain implementing HasParentLayout
(new interface) has the layout set, providing a way to get all the parameters in the layout chain.
Of course this is not finished yet (all router tests succeed, but a couple of other tests regarding route aliases for viewport and theme still fail), but I'm sharing this so if the feature doesn't get into 10 it might help others who need it from the start!
Any chance for this to be milestoned after 1.0 release?
This is becoming more pressing as time passes; I'll try to give a different point of view than the URL-based already discussed here.
Right now Flow assumes that parameterized views are leaves. This is quite restricting, since the subject identified by the parameter might need multiple independent views and provide links for them (for example, a customer view might have a view for details, orders, sold items, contacts, etc.). In cases like this, the parameterized view is also a router layout, providing navigation on its sub-views and possibly other common components.
This could be worked around using multiple sibling parameterized view, with links on each of them to each other. But this workaround lacks a fundamental aspect: the "main" parameterized view cannot share its state with its siblings as it would be if it were a router layout with its children.
I really hope this issue will be considered soon!
There is now an implementation available as extension to Vaadin Flow by @fluorumlabs : https://vaadin.com/directory/component/url-parameter-mapping
Thank you @samie for the link and @fluorumlabs for the great plugin! The POV I described before was about having parameterized router layouts, so to provide navigation links with the parameter to the sub-views. Do you think I need to open a new issue for that? I guess no, since it would also include this.
What's the status of this? Is this still planned for 13?
What's the status of this? Is this still planned for 13?
@heruan this is on the maybe list at the moment and to be honest it looks unlikely it will make it.
@pleku don't you think that readable hierarchical URLs are important? Is the task really so difficult to be implemented?
If there are some not obvious difficulties, please share. It can help with creating PR...
@remal I just checked the code, and I think the main problem is that this would require an almost full rewrite of RouteRegistry
, as its current structure only supports search by full path string.
@remal I've given a shot at this too (see https://github.com/vaadin/flow/compare/master...heruan:layout-url-parameters) and used that for a while successfully but I couldn't keep maintaining my fork so now is not up-to-date. Still, it could be a starting point if the team could give some feedback!
This is still blocking migration of some of our apps to Vaadin, any news about this? A version milestone maybe? 🙂
@heruan sorry not commiting to any milestones yet, but we have started to take a look "on the side" for making this happen in an upcoming minor release for Vaadin 14.
I can promise that after the feature is done, it will be available in the next minor release train for Vaadin 14, which should be released within no more than three months after the feature has been done 😎
Great news @pleku, thanks! Do you have design specs for this? I'm especially interested in the role of router layouts, and how parameters will be available on their instances.
We already have a quite smooth way for handling the common cases through the regular HasUrlParameter
interface. What we need is some way of supporting all other cases. From that point of view, flexibility and conceptual simplicity would be more important factors than e.g. type safety or avoiding magic strings. Interop with HasUrlParameter
is still relevant and one way of achieving that would be to use a design where HasUrlParameter
can be defined as a helper API on top of a more flexible low-level approach.
Based on those constraints, I would suggest placeholders in the @Route
value and a generic map-like abstraction for accessing typed placeholder values (e.g. parameters.getAsInt("id")
). The map would be available in all navigation events so that parent layouts also have easy access to the parameter values. The HasUrlParameter
value would be represented using a special key (e.g. _default
) in the map.
When it comes to the placeholder syntax in the @Route
value, I would favour the :
based syntax that is already used in e.g. our client-side router even though the URI Template syntax is a "real" standard.
To be decided which Vaadin 14 minor this lands into. Probably 14.4 even though window is still open for 14.3.
To be decided which Vaadin 14 minor this lands into. Probably 14.4 even though window is still open for 14.3.
It would be great if it could land in 14.3 .
Also to clarify here, this is now being shipped in 17.0.0 today, so please test it there. We would want to include this to 14-series as soon as possible, but we also don't want to ship anything in the LTS that is not "battle-proven".
If it works out well, dropping out a comment here will work and asking "when is this coming to 14" to let us know we need to get this in. EDIT: and just to clarify, unless there are severe issues discovered, 14.5 would be the current target as 14.4 is up next and it has been feature-freezed now.
When defining a routes map of an application, as a developer, I want to use routes where the route parameter is not the last segment of the URL (e.g.
/orders/:id/edit
). With the new Router API that is not supported at the moment (flow-1.0.0.alpha6).