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

History#replaceState trigger navigation event with React router #19613

Closed
leluna opened this issue Jun 20, 2024 · 14 comments · Fixed by #19736
Closed

History#replaceState trigger navigation event with React router #19613

leluna opened this issue Jun 20, 2024 · 14 comments · Fixed by #19736

Comments

@leluna
Copy link

leluna commented Jun 20, 2024

Description of the bug

When using React router, History#pushState and #replaceState trigger navigation event (trigger HISTORY, coming from a dom event "ui-navigate"). Even if only query parameters are changed.

With Flow Router, no navigation event occurs, not matter which part of the path has changed.

May relate to

Expected behavior

No navigation events on History#pushState and #replaceState. If a navigation event is desired, I would use UI.navigate.

Minimal reproducible example

I modified the example from a comment in #19580

@Route(value = "test/:path?")
public class TestView extends VerticalLayout implements BeforeEnterObserver, BeforeLeaveObserver {

    public TestView() {
        add(new Button("replace state - no query params",
                event -> UI.getCurrent().getPage().getHistory().replaceState(null, "test")));
        // no navigation only if the current URL is the same including query params
        add(new Button("replace state - push query param",
                event -> UI.getCurrent().getPage().getHistory().replaceState(null, "test?new")));
        // no navigation only if the current URL is the same including query params
        add(new Button("replace state - change path param",
                event -> UI.getCurrent().getPage().getHistory().replaceState(null, "test/new")));
        // no navigation only if the current URL is the same including query params
        add(new Button("push state - no query params",
                event -> UI.getCurrent().getPage().getHistory().pushState(null, "test")));
        // no navigation
        add(new Button("push state - query param",
                event -> UI.getCurrent().getPage().getHistory().pushState(null, "test?new")));
        // no navigation
        add(new Button("push state - change path param",
                event -> UI.getCurrent().getPage().getHistory().pushState(null, "test/new")));
        // no navigation
    }

    @Override
    public void beforeEnter(BeforeEnterEvent event) {
        add(new Div("beforeEnter"));
    }

    @Override
    public void beforeLeave(BeforeLeaveEvent beforeLeaveEvent) {
        add(new Div("beforeLeave"));
    }
}

Versions

  • Vaadin / Flow version: 24.4.3
  • Java version: 17
  • OS version: Windows
  • Browser version (if applicable):
  • Application Server (if applicable): Spring Boot
  • IDE (if applicable):
@alexanoid
Copy link

I can confirm that the following code:

ui.get().getPage().getHistory().replaceState(null, urlWithParameters);

cause the following issue: #19540

@mcollovati
Copy link
Collaborator

The issue was triaged and currently added to the backlog priority queue for further investigation

@alexanoid
Copy link

@mcollovati Thanks for looking into this. Is there any chance to get this fixed? I'm stuck on the prerelease version because of this issue and unable to upgrade to the stable version :(

@mcollovati
Copy link
Collaborator

We will start looking at this issue soon, but currently I cannot provide any estimate on when the bug will be fixed

@jameskerrtaskize
Copy link

This is blocking use from upgrading as well. Is there any update? Thanks

@jorgheymans
Copy link

@alexanoid what is the latest prerelease version that works for you? Looks like we'll need to downgrade as well then for a while.

@alexanoid
Copy link

@alexanoid what is the latest prerelease version that works for you? Looks like we'll need to downgrade as well then for a while.

24.4.0.beta5

@caalador
Copy link
Contributor

I see an issue with the no navigation functionality shown by the example also.
So now the route has a path parameter, but as there is no navigation event for the push state in

        add(new Button("push state - change path param",
                event -> UI.getCurrent().getPage().getHistory().pushState(null, "test/new")));
        // no navigation

we actually do not get the new path parameter to the server at all.
So the replaceState that sends the data to the server seems like the correct functionality as that gets us the parameter new where as for the pushState at the moment it doesn't and that doesn't seem correct.

for the query parameter change I would also expect to get it to the server, but the question is should all the events be fired (beforeEnter, beforeLeave, afterNavigation, setParameter) or should a navigation to the same target only execute beforeEnter and setParameter.

I feel we should get a comprehensive list of navigation expectations to get the full set working as expected instead of making exceptions here and there for the navigation.

@caalador
Copy link
Contributor

As a workaround if for some reason the url update needs to happen "outside" of all handling you can just call

JsonValue state = null;
String pathWithQueryParameters = "test/new";
ui.getPage().executeJs("setTimeout(() => { window.history.replaceState($0, '', $1);  })", state, pathWithQueryParameters);

But note that this may break forward/back navigation.

@leluna
Copy link
Author

leluna commented Jul 31, 2024

If I want to have all navigation events, I I would just use UI#navigate. Isn’t the whole point of the History class that it only pushes/replaces the state without firing events?

@caalador
Copy link
Contributor

Well it is used by the framework to handle url updates for navigation etc. so one one hand yes, but on the other hand just directly running the push and replace functions breaks the react-router and unknowingly allowing that through the api is not that good either.

If only using Flow and no Hilla/react then falling back to vaadin-router for now is a possibility by setting the vaadin.react.enable property to false.

@andersforsell
Copy link
Collaborator

I can confirm that in my Flow application getUI().get().getPage().getHistory().replaceState(null, deepLinkingUrl);
works fine again after setting vaadin.react.enable property to false.

caalador added a commit that referenced this issue Aug 1, 2024
Add feature to support replace and
push without getting a callback
to the server for the change.

fixes #19613
caalador added a commit that referenced this issue Aug 1, 2024
Add feature to support replace and
push without getting a callback
to the server for the change.

fixes #19613
@caalador caalador self-assigned this Aug 1, 2024
@caalador caalador moved this from 🔖 High Priority (P1) to 🏗 WIP in Vaadin Flow bugs & maintenance (Vaadin 10+) Aug 6, 2024
vaadin-bot pushed a commit that referenced this issue Aug 9, 2024
* feat: push and replace without navigation callback

Add feature to support replace and
push without getting a callback
to the server for the change.

fixes #19613

* change default callback for replace also to false

* history gets same events as previously with vaadin-router.

* Forward should callback to update url.

* Fix java doc link
vaadin-bot added a commit that referenced this issue Aug 9, 2024
* feat: push and replace without navigation callback

Add feature to support replace and
push without getting a callback
to the server for the change.

fixes #19613

* change default callback for replace also to false

* history gets same events as previously with vaadin-router.

* Forward should callback to update url.

* Fix java doc link

Co-authored-by: caalador <[email protected]>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.9.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.

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

Successfully merging a pull request may close this issue.

9 participants