Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize rerouteTo, forwardTo and navigate signatures #14553

Closed
OlliTietavainenVaadin opened this issue Sep 16, 2022 · 5 comments · Fixed by #17407
Closed

Synchronize rerouteTo, forwardTo and navigate signatures #14553

OlliTietavainenVaadin opened this issue Sep 16, 2022 · 5 comments · Fixed by #17407

Comments

@OlliTietavainenVaadin
Copy link
Member

Describe your motivation

Rough table of which types of parameters are accepted to each method:

<style type="text/css"></style>
T parameter = target interface HasUrlParameter type                    
  class class + T parameter class + T parameter + QueryParameters class + RouteParameters String String + QueryParameters NavigationState NavigationHandler + NavigationState String + List locationParams String + T parameter
navigate x x x x x x        
forwardTo x     x x   x x x x
rerouteTo x     x x   x x x x

forwardTo, rerouteTo, and navigate are all navigation methods and should therefore be usable consistently unless technically required to behave differently. So if any of them accepts a class as the navigation target, so should the others. If any one of them accepts a String to identify the target, so should the others. Most importantly, since navigate accepts route parameters being part of the location String, forwardTo and rerouteTo should also accept route parameters this way.

Describe the solution you'd like

There should be a better consistency of the signatures of the methods; at the very least, Query Parameters should be supported in every one of them.

Describe alternatives you've considered

Do we actually need three methods for in-app navigation? Could we drop one (or even two) of them?

@OlliTietavainenVaadin
Copy link
Member Author

Related issue: #14562

@TatuLund
Copy link
Contributor

Do we actually need three methods for in-app navigation? Could we drop one (or even two) of them?

It is a good question, where the answer is most likely yes.

UI#navigate starts complete navigation cycle, and calling UI#navigate while navigation cycle is not complete is not allowed. I.e. you are not supposed to use UI#navigate in beforeEnter for example, you can however call it in afterNavigation as in that stage navigation cycle is complete. Hence rerouteTo/forwardTo is necessary as you want to have have option to divert navigation while cycle is not complete yet. And this comes in two variants, either preserving the url or not. So if reducing number of methods it is theoretically possible to join rerouteTo/forwardTo by introducing additional parameter for url preservation feature. This naturally will be breaking change, which we probably should avoid introducing at this stage.

tltv added a commit that referenced this issue Aug 9, 2023
This change makes UI#navigate, BeforeEvent#rerouteTo and BeforeEvent#forwardTo methods more consistent without changing the existing API:
- adds new methods in BeforeEvent that accepts route parameters.
- adds new methods in BeforeEvent that accepts query parameters.
- improves forwardTo and rerouteTo to accept route parameters being part of the location String (same way as UI#navigate does it already).

Fixes: #14553
mshabarov pushed a commit that referenced this issue Aug 10, 2023
* feat: Update navigation method signatures

This change makes UI#navigate, BeforeEvent#rerouteTo and BeforeEvent#forwardTo methods more consistent without changing the existing API:
- adds new methods in BeforeEvent that accepts route parameters.
- adds new methods in BeforeEvent that accepts query parameters.
- improves forwardTo and rerouteTo to accept route parameters being part of the location String (same way as UI#navigate does it already).

Fixes: #14553

* Fixed javadoc

* Added hasRedirectQueryParameters()
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.2.0.alpha7 and is also targeting the upcoming stable 24.2.0 version.

vaadin-bot pushed a commit that referenced this issue Sep 22, 2023
* feat: Update navigation method signatures

This change makes UI#navigate, BeforeEvent#rerouteTo and BeforeEvent#forwardTo methods more consistent without changing the existing API:
- adds new methods in BeforeEvent that accepts route parameters.
- adds new methods in BeforeEvent that accepts query parameters.
- improves forwardTo and rerouteTo to accept route parameters being part of the location String (same way as UI#navigate does it already).

Fixes: #14553

* Fixed javadoc

* Added hasRedirectQueryParameters()
vaadin-bot pushed a commit that referenced this issue Sep 22, 2023
* feat: Update navigation method signatures

This change makes UI#navigate, BeforeEvent#rerouteTo and BeforeEvent#forwardTo methods more consistent without changing the existing API:
- adds new methods in BeforeEvent that accepts route parameters.
- adds new methods in BeforeEvent that accepts query parameters.
- improves forwardTo and rerouteTo to accept route parameters being part of the location String (same way as UI#navigate does it already).

Fixes: #14553

* Fixed javadoc

* Added hasRedirectQueryParameters()
vaadin-bot added a commit that referenced this issue Sep 22, 2023
* feat: Update navigation method signatures

This change makes UI#navigate, BeforeEvent#rerouteTo and BeforeEvent#forwardTo methods more consistent without changing the existing API:
- adds new methods in BeforeEvent that accepts route parameters.
- adds new methods in BeforeEvent that accepts query parameters.
- improves forwardTo and rerouteTo to accept route parameters being part of the location String (same way as UI#navigate does it already).

Fixes: #14553

* Fixed javadoc

* Added hasRedirectQueryParameters()

Co-authored-by: Tomi Virtanen <[email protected]>
mcollovati pushed a commit that referenced this issue Sep 22, 2023
This change makes UI#navigate, BeforeEvent#rerouteTo and BeforeEvent#forwardTo methods more consistent without changing the existing API:
- adds new methods in BeforeEvent that accepts route parameters.
- adds new methods in BeforeEvent that accepts query parameters.
- improves forwardTo and rerouteTo to accept route parameters being part of the location String (same way as UI#navigate does it already).

Fixes: #14553
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.3.25.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.1.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants