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: multiple fast navigate calls #20446

Merged
merged 8 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
useBlocker,
useLocation,
useNavigate,
type NavigateOptions,
type NavigateOptions, useHref,
} from "react-router-dom";
import type { AgnosticRouteObject } from '@remix-run/router';
import { createPortal } from "react-dom";
Expand Down Expand Up @@ -273,10 +273,12 @@ function Flow() {
});
const location = useLocation();
const navigated = useRef<boolean>(false);
const blockerHandled = useRef<boolean>(false);
const fromAnchor = useRef<boolean>(false);
const containerRef = useRef<RouterContainer | undefined>(undefined);
const roundTrip = useRef<Promise<void> | undefined>(undefined);
const queuedNavigate = useQueuedNavigate(roundTrip, navigated);
const basename = useHref('/');

// portalsReducer function is used as state outside the Flow component.
const [portals, dispatchPortalAction] = useReducer(portalsReducer, []);
Expand Down Expand Up @@ -361,8 +363,18 @@ function Flow() {

useEffect(() => {
if (blocker.state === 'blocked') {
if(blockerHandled.current) {
// Blocker is handled and the new navigation
// gets queued to be executed after the current handling ends.
const {pathname, state} = blocker.location;
queuedNavigate(pathname.substring(basename.length), true, { state: state, replace: true });
return;
}
blockerHandled.current = true;
let blockingPromise: any;
roundTrip.current = new Promise<void>((resolve,reject) => blockingPromise = {resolve:resolve,reject:reject});
// Release blocker handling after promise is fulfilled
roundTrip.current.then(() => blockerHandled.current = false, () => blockerHandled.current = false);

// Proceed to the blocked location, unless the navigation originates from a click on a link.
// In that case continue with function execution and perform a server round-trip
Expand Down
12 changes: 12 additions & 0 deletions flow-tests/test-react-router/pom-production.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>com.vaadin</groupId>
<artifactId>flow-react</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.seleniumhq.selenium</groupId>
<artifactId>selenium-chrome-driver</artifactId>
<version>4.26.0</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
13 changes: 13 additions & 0 deletions flow-tests/test-react-router/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,19 @@
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>com.vaadin</groupId>
<artifactId>flow-react</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>org.seleniumhq.selenium</groupId>
<artifactId>selenium-chrome-driver</artifactId>
<version>4.26.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worrying about maintenance for this: future Chrome updates can be incompatible with the ChromeDriver, so we have to bump this version all the time.
If the test always passes, maybe we can disable it, so it will show case the failing scenario, but won't run in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed test and added doc on the view class.

<scope>test</scope>
</dependency>

</dependencies>

<build>
Expand Down
22 changes: 22 additions & 0 deletions flow-tests/test-react-router/src/main/frontend/NavigateView.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import {useNavigate} from "react-router-dom";
import {
ReactAdapterElement,
RenderHooks
} from "Frontend/generated/flow/ReactAdapter";

class NavigateView extends ReactAdapterElement {
protected render(hooks: RenderHooks): React.ReactElement | null {
const navigate = useNavigate();

return (
<>
<p id="react">This is a simple view for a React route</p>
<button id="react-navigate" onClick={() => navigate("com.vaadin.flow.RouterView"!)}>
Navigate button
</button>
</>
);
}
}

customElements.define('navigate-view', NavigateView);
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2000-2024 Vaadin Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/

package com.vaadin.flow;

import com.vaadin.flow.component.Tag;
import com.vaadin.flow.component.dependency.JsModule;
import com.vaadin.flow.component.react.ReactAdapterComponent;
import com.vaadin.flow.router.Route;

@Route("com.vaadin.flow.ReactNavigateView")
@Tag("navigate-view")
@JsModule("NavigateView.tsx")
public class ReactNavigateView extends ReactAdapterComponent {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright 2000-2024 Vaadin Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/

package com.vaadin.flow;

import java.io.IOException;
import java.time.Duration;
import java.util.HashMap;
import java.util.Map;

import com.google.common.collect.ImmutableMap;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.openqa.selenium.chrome.ChromeDriver;
import org.openqa.selenium.remote.CommandExecutor;
import org.openqa.selenium.support.ui.ExpectedConditions;
import org.openqa.selenium.remote.Command;

import com.vaadin.flow.component.html.testbench.NativeButtonElement;
import com.vaadin.flow.testutil.ChromeBrowserTest;
import com.vaadin.testbench.TestBenchDriverProxy;

public class ReactNavigateIT extends ChromeBrowserTest {

@Test
public void testSlowNavigation_clickReactNavigateButtonTwice_noExceptionInLogs()
throws IOException {
open();
ChromeDriver driver = (ChromeDriver) ((TestBenchDriverProxy) getDriver())
.getWrappedDriver();

CommandExecutor executor = driver.getCommandExecutor();
Map map = new HashMap();
map.put("offline", false);
map.put("latency", 100);

map.put("download_throughput", 1800);
map.put("upload_throughput", 400);

executor.execute(new Command(driver.getSessionId(),
"setNetworkConditions", ImmutableMap.of("network_conditions",
ImmutableMap.copyOf(map))));

waitUntil(drvr -> $(NativeButtonElement.class).id("react-navigate")
.isDisplayed());

NativeButtonElement navigateButton = $(NativeButtonElement.class)
.id("react-navigate");

// timout and requestAnimationFrame will give an undefined for the
// button.
driver.executeScript("arguments[0].click();arguments[0].click();",
navigateButton);

waitUntil(ExpectedConditions.stalenessOf(navigateButton));

checkLogsForErrors();

}

}
Loading