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

fix: queued navigate with React Router #19985

Merged
merged 11 commits into from
Sep 30, 2024
Merged

Conversation

platosha
Copy link
Contributor

Fixes #19839

Copy link

github-actions bot commented Sep 19, 2024

Test Results

1 139 files  + 2  1 139 suites  +2   1h 10m 2s ⏱️ + 2m 16s
7 405 tests + 6  7 355 ✅ + 6  50 💤 ±0  0 ❌ ±0 
7 735 runs   - 31  7 677 ✅  - 29  58 💤  - 2  0 ❌ ±0 

Results for commit ad8f36d. ± Comparison against base commit 8b7f0b1.

♻️ This comment has been updated with latest results.

@caalador
Copy link
Contributor

Seems that when we are not logged in and click the Navigate to admin view in 1 second button in the tests we do navigate to /admin then /my/login/page which seems fine but then we get one more navigate to /my/login/page which does a proceed on the blocker even though we have a server roundtrip going which then calls proceed again after the last call already let the blocker proceed and is already in the unblocked state

@caalador
Copy link
Contributor

Actually it seems it is /admin goes to the server and then /my/login/page releases the blocker before the /admin returns. Blocking the last my/login call still seems to create the blocker state fault.

@caalador
Copy link
Contributor

Just as a test dopping any blocker requests when a server roundtrip is waiting does fix the error, but I could expect this to have some drawbacks for other requests in the set requiring a roundtrip when multiple changes are made

diff --git forkSrcPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx forkDstPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx
index b073570de0e7926bcdb2b37ec34abc815e8d1297..c03527c624525b9b362e96e8570510344f197bbb 100644
--- forkSrcPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx
+++ forkDstPrefix/flow-server/src/main/resources/com/vaadin/flow/server/frontend/Flow.tsx
@@ -257,6 +257,7 @@ function Flow() {
     const {pathname, search, hash} = useLocation();
     const navigated = useRef<boolean>(false);
     const fromAnchor = useRef<boolean>(false);
+    const serverRoundtrip = useRef<boolean>(false);
     const containerRef = useRef<RouterContainer | undefined>(undefined);
     const queuedNavigate = useQueuedNavigate();
 
@@ -306,11 +307,15 @@ function Flow() {
     const vaadinNavigateEventHandler = useCallback((event: CustomEvent<{state: unknown, url: string, replace?: boolean, callback: boolean}>) => {
         const path = '/' + event.detail.url;
         navigated.current = !event.detail.callback;
+        if(event.detail.replace && path === window.location.pathname) {
+            return;
+        }
         queuedNavigate(path, { state: event.detail.state, replace: event.detail.replace});
     }, [navigate]);
 
     const redirect = useCallback((path: string) => {
         return (() => {
+            serverRoundtrip.current = false;
             navigate(path, {replace: true});
         });
     }, [navigate]);
@@ -339,12 +344,16 @@ function Flow() {
 
     useEffect(() => {
         if (blocker.state === 'blocked') {
+            if(serverRoundtrip.current) {
+                return;
+            }
             // Do not skip server round-trip if navigation originates from a click on a link
             if (navigated.current && !fromAnchor.current) {
                 blocker.proceed();
                 return;
             }
             fromAnchor.current = false;
+            serverRoundtrip.current = true;
             const {pathname, search} = blocker.location;
             const routes = ((window as any)?.Vaadin?.routesConfig || []) as AgnosticRouteObject[];
             let matched = matchRoutes(Array.from(routes), pathname);
@@ -357,10 +366,12 @@ function Flow() {
                         prevent() {
                             blocker.reset();
                             navigated.current = false;
+                            serverRoundtrip.current = false;
                         },
                         redirect,
                         continue() {
                             blocker.proceed();
+                            serverRoundtrip.current = false;
                         }
                     }, router);
                 navigated.current = true;
@@ -376,13 +387,16 @@ function Flow() {
                             containerRef.current.serverConnected = (cancel) => {
                                 if (cancel) {
                                     blocker.reset();
+                                    serverRoundtrip.current = false;
                                 } else {
                                     blocker.proceed();
+                                    serverRoundtrip.current = false;
                                 }
                             }
                         } else {
                             // permitted navigation: proceed with the blocker
                             blocker.proceed();
+                            serverRoundtrip.current = false;
                         }
                     });
             }

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Sep 23, 2024
Block navigation to wait for
response from the server if
a roundtrip is made.
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Sep 23, 2024
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Sep 23, 2024
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Sep 26, 2024
caalador
caalador previously approved these changes Sep 26, 2024
@caalador caalador marked this pull request as ready for review September 26, 2024 06:52
Copy link

sonarcloud bot commented Sep 27, 2024

@caalador caalador merged commit 9ceb7d7 into main Sep 30, 2024
26 checks passed
@caalador caalador deleted the fix/ap/react-history-async branch September 30, 2024 04:38
vaadin-bot pushed a commit that referenced this pull request Sep 30, 2024
Fixes #19839

---------

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

Hi @platosha and @caalador, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 9ceb7d7
error: could not apply 9ceb7d7... fix: queued navigate with React Router (#19985)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

vaadin-bot added a commit that referenced this pull request Sep 30, 2024
Fixes #19839

---------

Co-authored-by: Anton Platonov <[email protected]>
Co-authored-by: caalador <[email protected]>
Co-authored-by: Teppo Kurki <[email protected]>
caalador added a commit that referenced this pull request Sep 30, 2024
Fixes #19839

---------

Co-authored-by: caalador <[email protected]>
Co-authored-by: Teppo Kurki <[email protected]>
mcollovati pushed a commit that referenced this pull request Sep 30, 2024
Fixes #19839

---------

Co-authored-by: Anton Platonov <[email protected]>
Co-authored-by: Teppo Kurki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser back navigation does not work when using replaceState with React enabled
5 participants