⬆️ Upgrade to React 18, migrate to React Router 6

Why: - We were using a pre-release version of React concurrent mode, but it has already been released officially, so it's time to upgrade. - React's StrictMode causes the elements to be mounted twice, which caused two the OpenLayers Maps to be created inside the div. The fix was to implement componentWillUnmount and destroy the Map on unmount. - Reach Router is incompatible with React 18, and StrictMode causes navigation to not work. There will be no Reach Router v2, so the fix is to migrate to React Router v6. reach/router#504 https://reactrouter.com/en/main/upgrading/reach - React Router doesn't have the equivalent of Reach Router's globalHistory, so we used the useEffect workaround from remix-run/remix#5999
luontola · Jul 31, 2023 · 7d0a9e2 · 7d0a9e2
1 parent b59719e
commit 7d0a9e2
Show file tree

Hide file tree

Showing 21 changed files with 209 additions and 135 deletions.
diff --git a/package.json b/package.json
@@ -12,7 +12,6 @@
   },
   "dependencies": {
     "@luontola/react-cache": "^2.0.0-alpha.2019-05-02",
-    "@reach/router": "^1.3.3",
     "alphanum-sort": "^1.0.2",
     "auth0-js": "^9.13.2",
     "axios": "^0.19.0",
@@ -26,11 +25,12 @@
     "ol": "^6.3.0",
     "prop-types": "^15.7.2",
     "purecss": "^1.0.1",
-    "react": "^0.0.0-experimental-e5d06e34b",
-    "react-dom": "^0.0.0-experimental-e5d06e34b",
+    "react": "^18.2.0",
+    "react-dom": "^18.2.0",
     "react-error-boundary": "^1.2.5",
     "react-intl": "^4.3.1",
     "react-qr-svg": "^2.2.2",
+    "react-router-dom": "^6.14.2",
     "regenerator-runtime": "^0.13.5"
   },
   "devDependencies": {
@@ -47,6 +47,7 @@
     "@storybook/react": "^5.3.18",
     "@types/auth0-js": "^9.12.4",
     "@types/lodash": "^4.14.149",
+    "@types/react-dom": "^18.2.7",
     "babel-loader": "^8.1.0",
     "babel-plugin-react-intl": "^7.1.0",
     "babel-preset-react-app": "^9.1.2",

diff --git a/territory-bro.iml b/territory-bro.iml
@@ -16,6 +16,9 @@
       <sourceFolder url="file://$MODULE_DIR$/test" isTestSource="true" />
       <excludeFolder url="file://$MODULE_DIR$/target" />
       <excludeFolder url="file://$MODULE_DIR$/target/default" />
+      <excludeFolder url="file://$MODULE_DIR$/.clj-kondo" />
+      <excludeFolder url="file://$MODULE_DIR$/.lsp" />
+      <excludeFolder url="file://$MODULE_DIR$/.yarn" />
     </content>
     <orderEntry type="inheritedJdk" />
     <orderEntry type="sourceFolder" forTests="false" />

diff --git a/web/js/index.tsx b/web/js/index.tsx
@@ -2,9 +2,8 @@
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
-import React from "react";
-import ReactDOM from "react-dom";
-import {globalHistory, Router} from "@reach/router";
+import React, {useEffect} from "react";
+import ReactDOM from "react-dom/client";
 import {IntlProvider} from "react-intl";
 import {language, messages} from "./intl";
 import ErrorBoundary from "react-error-boundary";
@@ -25,18 +24,19 @@ import TerritoryListPage from "./pages/TerritoryListPage";
 import TerritoryPage from "./pages/TerritoryPage";
 import OpenSharePage from "./pages/OpenSharePage";
 import {getPageState, setPageState} from "./util";
+import {BrowserRouter, Route, Routes, useLocation} from "react-router-dom";
 
-let previousLocation = globalHistory.location;
-globalHistory.listen(({location, action}) => {
-  if (previousLocation.href === location.href) {
-    return; // only page state was updated, but the URL remains the same
-  }
-  previousLocation = location;
-
-  console.info(`Current URL is now ${location.pathname}${location.search}${location.hash} (${action})`);
-  logPageView();
+function NavigationListener({children}) {
+  const location = useLocation()
+  useEffect(() => {
+    console.info(`Current URL is now ${location.pathname}${location.search}${location.hash}`);
+    logPageView();
+    restoreScrollY(getPageState("scrollY"));
+  }, [location.pathname])
+  return children;
+}
 
-  const scrollY = getPageState("scrollY");
+function restoreScrollY(scrollY?: number) {
   if (typeof scrollY === 'number') {
     // XXX: scrolling immediately does not scroll far enough; maybe we must wait for React to render the whole page
     setTimeout(() => {
@@ -45,7 +45,7 @@ globalHistory.listen(({location, action}) => {
   } else {
     document.querySelector('main')?.scrollIntoView({block: "start"});
   }
-});
+}
 
 function listenScrollY(onScroll) {
   let lastKnownScrollY = 0;
@@ -75,27 +75,32 @@ ReactDOM.createRoot(document.getElementById('root'))
     <React.StrictMode>
       <ErrorBoundary FallbackComponent={ErrorPage}>
         <IntlProvider locale={language} messages={messages}>
-          <React.Suspense fallback={<p>Loading....</p>}>
-            <Layout>
-              <Router>
-                <HomePage path="/"/>
-                <JoinPage path="/join"/>
-                <LoginCallbackPage path="/login-callback"/>
-                <RegistrationPage path="/register"/>
-                <HelpPage path="/help"/>
-                <OpenSharePage path="/share/:shareKey/*"/>
+          <BrowserRouter>
+            <NavigationListener>
+              <React.Suspense fallback={<p>Loading....</p>}>
+                <Layout>
+                  <Routes>
+                    <Route path="/" element={<HomePage/>}/>
+                    <Route path="/join" element={<JoinPage/>}/>
+                    <Route path="/login-callback" element={<LoginCallbackPage/>}/>
+                    <Route path="/register" element={<RegistrationPage/>}/>
+                    <Route path="/help" element={<HelpPage/>}/>
+                    <Route path="/share/:shareKey/*" element={<OpenSharePage/>}/>
 
-                <CongregationPage path="/congregation/:congregationId"/>
-                <TerritoryListPage path="/congregation/:congregationId/territories"/>
-                <TerritoryPage path="/congregation/:congregationId/territories/:territoryId"/>
-                <PrintoutPage path="/congregation/:congregationId/printouts"/>
-                <SettingsPage path="/congregation/:congregationId/settings"/>
-                <UsersPage path="/congregation/:congregationId/users"/>
+                    <Route path="/congregation/:congregationId" element={<CongregationPage/>}/>
+                    <Route path="/congregation/:congregationId/territories" element={<TerritoryListPage/>}/>
+                    <Route path="/congregation/:congregationId/territories/:territoryId" element={<TerritoryPage/>}/>
+                    <Route path="/congregation/:congregationId/printouts" element={<PrintoutPage/>}/>
+                    <Route path="/congregation/:congregationId/settings" element={<SettingsPage/>}/>
+                    <Route path="/congregation/:congregationId/users" element={<UsersPage/>}/>
 
-                <NotFoundPage default/>
-              </Router>
-            </Layout>
-          </React.Suspense>
+                    <Route path="*" element={<NotFoundPage/>}/>
+                  </Routes>
+                </Layout>
+              </React.Suspense>
+            </NavigationListener>
+          </BrowserRouter>
         </IntlProvider>
       </ErrorBoundary>
-    </React.StrictMode>);
+    </React.StrictMode>
+  );
diff --git a/web/js/layout/Layout.tsx b/web/js/layout/Layout.tsx
@@ -1,4 +1,4 @@
-// Copyright © 2015-2020 Esko Luontola
+// Copyright © 2015-2023 Esko Luontola
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
@@ -7,18 +7,11 @@ import styles from "./Layout.css";
 import * as React from "react";
 import {useEffect} from "react";
 import AuthenticationPanel from "./AuthenticationPanel";
-import {Link, Router} from "@reach/router";
 import {getCongregationById} from "../api";
+import {NavLink as RouterNavLink, Route, Routes, useParams} from "react-router-dom";
 
 const NavLink = (props) => (
-  <Link {...props}
-        getProps={({href, isCurrent, isPartiallyCurrent}) => {
-          // the object returned here is passed to the anchor element's props
-          const active = (href === '/') ? isCurrent : isPartiallyCurrent;
-          return {
-            className: active ? styles.active : undefined
-          };
-        }}/>
+  <RouterNavLink className={({isActive}) => isActive ? styles.active : ""} {...props}/>
 );
 
 const HomeNav = ({}) => {
@@ -34,15 +27,16 @@ const HomeNav = ({}) => {
   );
 }
 
-const CongregationNav = ({congregationId}) => {
+const CongregationNav = () => {
+  const {congregationId} = useParams()
   const congregation = getCongregationById(congregationId);
   return (
     <ul className={styles.nav}>
       <li><NavLink to="/">Home</NavLink></li>
       <li><NavLink to=".">{congregation.name}</NavLink></li>
       <li><NavLink to="territories">Territories</NavLink></li>
       {congregation.permissions.viewCongregation &&
-      <li><NavLink to="printouts">Printouts</NavLink></li>}
+        <li><NavLink to="printouts">Printouts</NavLink></li>}
       {congregation.permissions.configureCongregation && <>
         <li><NavLink to="users">Users</NavLink></li>
         <li><NavLink to="settings">Settings</NavLink></li>
@@ -52,12 +46,6 @@ const CongregationNav = ({congregationId}) => {
   );
 }
 
-function RouterComponent({children}) {
-  // Workaround for Reach Router to not render in a <div> which messes up flexbox.
-  // See https://github.com/reach/router/issues/63#issuecomment-524297867
-  return <>{children}</>;
-}
-
 type Props = {
   title?: string;
   children?: React.ReactNode;
@@ -72,10 +60,10 @@ const Layout = ({title, children}: Props) => {
 
   return <>
     <nav className={`${styles.navbar} no-print`}>
-      <Router primary={false} component={RouterComponent}>
-        <HomeNav path="/*"/>
-        <CongregationNav path="/congregation/:congregationId/*"/>
-      </Router>
+      <Routes>
+        <Route path="/*" element={<HomeNav/>}/>
+        <Route path="/congregation/:congregationId/*" element={<CongregationNav/>}/>
+      </Routes>
       <div className={styles.auth}>
         <AuthenticationPanel/>
       </div>

diff --git a/web/js/maps/NeighborhoodMap.ts b/web/js/maps/NeighborhoodMap.ts
@@ -1,4 +1,4 @@
-// Copyright © 2015-2020 Esko Luontola
+// Copyright © 2015-2023 Esko Luontola
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
@@ -35,7 +35,7 @@ export default class NeighborhoodMap extends OpenLayersMap<Props> {
       territory,
       mapRaster
     } = this.props;
-    this.map = initNeighborhoodMap(this.element, territory);
+    this.map = initNeighborhoodMap(this.elementRef.current, territory);
     this.map.setStreetsLayerRaster(mapRaster);
   }
 
@@ -45,6 +45,10 @@ export default class NeighborhoodMap extends OpenLayersMap<Props> {
     } = this.props;
     this.map.setStreetsLayerRaster(mapRaster);
   }
+
+  componentWillUnmount() {
+    this.map.unmount()
+  }
 }
 
 function initNeighborhoodMap(element: HTMLDivElement, territory: Territory): any {
@@ -85,6 +89,9 @@ function initNeighborhoodMap(element: HTMLDivElement, territory: Territory): any
   return {
     setStreetsLayerRaster(mapRaster: MapRaster): void {
       streetsLayer.setSource(mapRaster.source);
+    },
+    unmount() {
+      map.setTarget(undefined)
     }
   };
 }
diff --git a/web/js/maps/OpenLayersMap.tsx b/web/js/maps/OpenLayersMap.tsx
@@ -1,4 +1,4 @@
-// Copyright © 2015-2020 Esko Luontola
+// Copyright © 2015-2023 Esko Luontola
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
@@ -12,19 +12,19 @@ type Props = {
 
 export default class OpenLayersMap<P extends Props> extends React.Component<P> {
 
-  element: HTMLDivElement;
+  elementRef: React.RefObject<HTMLDivElement>;
+
+  constructor(props: P) {
+    super(props);
+    this.elementRef = React.createRef()
+  }
 
   render() {
     const {printout} = this.props;
     let className = styles.root;
     if (printout) {
       className += " " + styles.printout;
     }
-    const setElement = el => {
-      if (el) {
-        this.element = el;
-      }
-    };
-    return <div className={className} ref={setElement}/>;
+    return <div className={className} ref={this.elementRef}/>;
   }
 }
diff --git a/web/js/maps/RegionMap.ts b/web/js/maps/RegionMap.ts
@@ -1,4 +1,4 @@
-// Copyright © 2015-2020 Esko Luontola
+// Copyright © 2015-2023 Esko Luontola
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
@@ -38,7 +38,7 @@ export default class RegionMap extends OpenLayersMap<Props> {
       territories,
       mapRaster
     } = this.props;
-    this.map = initRegionMap(this.element, region, territories);
+    this.map = initRegionMap(this.elementRef.current, region, territories);
     this.map.setStreetsLayerRaster(mapRaster);
   }
 
@@ -48,6 +48,10 @@ export default class RegionMap extends OpenLayersMap<Props> {
     } = this.props;
     this.map.setStreetsLayerRaster(mapRaster);
   }
+
+  componentWillUnmount() {
+    this.map.unmount()
+  }
 }
 
 function initRegionMap(element: HTMLDivElement, region: Region | Congregation, territories: Array<Territory>): any {
@@ -103,6 +107,9 @@ function initRegionMap(element: HTMLDivElement, region: Region | Congregation, t
   return {
     setStreetsLayerRaster(mapRaster: MapRaster): void {
       streetsLayer.setSource(mapRaster.source);
+    },
+    unmount() {
+      map.setTarget(undefined)
     }
   };
 }
diff --git a/web/js/maps/TerritoryListMap.ts b/web/js/maps/TerritoryListMap.ts
@@ -1,4 +1,4 @@
-// Copyright © 2015-2022 Esko Luontola
+// Copyright © 2015-2023 Esko Luontola
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
@@ -39,7 +39,7 @@ export default class TerritoryListMap extends OpenLayersMap<Props> {
       territories,
       onClick,
     } = this.props;
-    this.map = initMap(this.element, congregation, territories, onClick);
+    this.map = initMap(this.elementRef.current, congregation, territories, onClick);
   }
 
   componentDidUpdate() {
@@ -48,6 +48,10 @@ export default class TerritoryListMap extends OpenLayersMap<Props> {
     } = this.props;
     this.map.updateTerritories(territories);
   }
+
+  componentWillUnmount() {
+    this.map.unmount()
+  }
 }
 
 function loanableTerritoryStroke(loaned) {
@@ -177,6 +181,9 @@ function initMap(element: HTMLDivElement,
     updateTerritories(territories: Array<Territory>): void {
       setTerritories(territories);
       resetZoom(map, {duration: 300});
+    },
+    unmount() {
+      map.setTarget(undefined)
     }
   };
 }
diff --git a/web/js/maps/TerritoryMap.ts b/web/js/maps/TerritoryMap.ts
@@ -1,4 +1,4 @@
-// Copyright © 2015-2020 Esko Luontola
+// Copyright © 2015-2023 Esko Luontola
 // This software is released under the Apache License 2.0.
 // The license text is at http://www.apache.org/licenses/LICENSE-2.0
 
@@ -40,7 +40,7 @@ export default class TerritoryMap extends OpenLayersMap<Props> {
       mapRaster,
       printout,
     } = this.props;
-    this.map = initTerritoryMap(this.element, territory, printout);
+    this.map = initTerritoryMap(this.elementRef.current, territory, printout);
     this.map.setStreetsLayerRaster(mapRaster);
   }
 
@@ -50,6 +50,10 @@ export default class TerritoryMap extends OpenLayersMap<Props> {
     } = this.props;
     this.map.setStreetsLayerRaster(mapRaster);
   }
+
+  componentWillUnmount() {
+    this.map.unmount()
+  }
 }
 
 function startGeolocation(map) {
@@ -129,6 +133,9 @@ function initTerritoryMap(element: HTMLDivElement, territory: Territory, printou
   return {
     setStreetsLayerRaster(mapRaster: MapRaster): void {
       streetsLayer.setSource(mapRaster.source);
+    },
+    unmount() {
+      map.setTarget(undefined)
     }
   };
 }