Browse Source

feat(rr6): Remove react-router 3 components (#78350)

We don't need these anymore, we just need the shims
Evan Purkhiser 5 months ago
parent
commit
676339a6b0

+ 28 - 73
static/app/components/route.tsx

@@ -1,16 +1,13 @@
-// biome-ignore lint/nursery/noRestrictedImports: Will be removed with react router 6
-import type {IndexRouteProps, PlainRoute, RouteProps} from 'react-router';
-// biome-ignore lint/nursery/noRestrictedImports: Will be removed with react router 6
-import {IndexRoute as BaseIndexRoute, Route as BaseRoute} from 'react-router';
+import type {
+  IndexRedirectProps,
+  IndexRouteProps,
+  RedirectProps,
+  RouteProps,
+} from 'sentry/types/legacyReactRouter';
 
-import {USING_CUSTOMER_DOMAIN} from 'sentry/constants';
-import withDomainRedirect from 'sentry/utils/withDomainRedirect';
-import withDomainRequired from 'sentry/utils/withDomainRequired';
-
-// This module contains customized react-router route components used to
-// construct the app routing tree.
-//
-// The primary customization here relates to supporting rendering dual-routes for customer domains
+// This module contains the "fake" react components that are used as we migrade
+// off of react-router 3 to 6. The shims in the utils/reactRouter6Compat module
+// read the props off tese components and construct a real react router 6 tree.
 
 type CustomProps = {
   /**
@@ -34,68 +31,26 @@ type CustomProps = {
 
 interface SentryRouteProps extends React.PropsWithChildren<RouteProps & CustomProps> {}
 
-type RouteElement = React.ReactElement<SentryRouteProps>;
-
-// The original createRouteFromReactElement extracted from the base route. This
-// is not properly typed hence the ts-ignore.
-//
-// @ts-ignore
-const createRouteFromReactElement = BaseRoute.createRouteFromReactElement;
-
-/**
- * Customized React Router Route configuration component.
- */
-const Route = BaseRoute as React.ComponentClass<SentryRouteProps>;
-
-// We override the createRouteFromReactElement property to provide support for
-// the withOrgPath property.
-//
-// XXX(epurkhiser): It is important to note! The `Route` component is a
-// CONFIGURATION ONLY COMPONENT. It DOES NOT render! This function is part of
-// the react-router magic internals that are used to build the route tree by
-// traversing the component tree, that is why this logic lives here and not
-// inside a custom Route component.
-//
-// To understand deeper how this works, see [0].
-//
-// When `withOrgPath` is provided to the Route configuration component the
-// react-router router builder will use this function which splits the single
-// Route into two, one for the route with :orgId and one for the new-style
-// route.
-//
-// [0]: https://github.com/remix-run/react-router/blob/850de933444d260bfc5460135d308f9d74b52c97/modules/RouteUtils.js#L15
-//
-// @ts-ignore
-Route.createRouteFromReactElement = function (element: RouteElement): PlainRoute {
-  const {withOrgPath, component, path} = element.props;
-
-  if (!withOrgPath) {
-    return createRouteFromReactElement(element);
-  }
-
-  const childRoutes: PlainRoute[] = [
-    {
-      ...createRouteFromReactElement(element),
-      path: `/organizations/:orgId${path}`,
-      component: withDomainRedirect(component ?? NoOp),
-    },
-  ];
-
-  if (USING_CUSTOMER_DOMAIN) {
-    childRoutes.unshift({
-      ...createRouteFromReactElement(element),
-      path,
-      component: withDomainRequired(component ?? NoOp),
-    });
-  }
-
-  return {childRoutes};
-};
+export function Route(_props: SentryRouteProps) {
+  // XXX: These routes are NEVER rendered
+  return null;
+}
+Route.displayName = 'Route';
 
-function NoOp({children}: {children: JSX.Element}) {
-  return children;
+export function IndexRoute(_props: IndexRouteProps & CustomProps) {
+  // XXX: These routes are NEVER rendered
+  return null;
 }
+IndexRoute.displayName = 'IndexRoute';
 
-const IndexRoute = BaseIndexRoute as React.ComponentClass<IndexRouteProps & CustomProps>;
+export function Redirect(_props: RedirectProps) {
+  // XXX: These routes are NEVER rendered
+  return null;
+}
+Redirect.displayName = 'Redirect';
 
-export {Route, IndexRoute};
+export function IndexRedirect(_props: IndexRedirectProps) {
+  // XXX: These routes are NEVER rendered
+  return null;
+}
+IndexRedirect.displayName = 'IndexRedirect';

+ 29 - 37
static/app/routes.spec.tsx

@@ -1,9 +1,8 @@
-// biome-ignore lint/nursery/noRestrictedImports: Will be removed with react router 6
-import {createRoutes} from 'react-router';
+import type {RouteObject} from 'react-router-dom';
 
 import * as constants from 'sentry/constants';
 import {buildRoutes} from 'sentry/routes';
-import type {RouteComponent} from 'sentry/types/legacyReactRouter';
+import {buildReactRouter6Routes} from 'sentry/utils/reactRouter6Compat/router';
 import normalizeUrl from 'sentry/utils/url/normalizeUrl';
 
 // Setup a module mock so that we can replace
@@ -20,58 +19,51 @@ jest.mock('sentry/constants', () => {
   };
 });
 
-// Workaround react-router PlainRoute type not covering redirect routes.
-type RouteShape = {
-  childRoutes?: RouteShape[];
-  component?: RouteComponent;
-  from?: string;
-  path?: string;
-};
-
 type RouteMetadata = {
   leadingPath: string;
-  route: RouteShape;
+  route: RouteObject;
 };
 
-function extractRoutes(rootRoute: any): Record<string, RouteComponent> {
-  const routeTree = createRoutes(rootRoute);
-  const routeMap: Record<string, RouteComponent> = {};
+function extractRoutes(rootRoute: RouteObject[]): Set<string> {
+  const routes = new Set<string>();
 
   // A queue of routes we need to visit
-  const visitQueue: RouteMetadata[] = [{leadingPath: '', route: routeTree[0]}];
+  const visitQueue: RouteMetadata[] = [{leadingPath: '', route: rootRoute[0]}];
   while (visitQueue.length > 0) {
     const current = visitQueue.pop();
     if (!current) {
       break;
     }
-    let leading = current.leadingPath;
-    if (current.route.path?.startsWith('/')) {
-      leading = '';
-    }
+    const leading = current.leadingPath;
 
     const currentPath = `${leading}${current.route.path ?? ''}`.replace('//', '/');
-    if (current.route.childRoutes) {
-      for (const childRoute of current.route.childRoutes ?? []) {
+    if (current.route.children) {
+      for (const childRoute of current.route.children ?? []) {
         visitQueue.push({
           leadingPath: currentPath,
           route: childRoute,
         });
       }
-    } else {
-      if (current.route.from) {
-        // Redirect routes are not relevant to us.
-        continue;
-      }
+    }
 
-      // We are on a terminal route in the tree. Add to the map of route components.
-      // We are less interested in container route components.
-      if (current.route.component) {
-        routeMap[currentPath] = current.route.component;
-      }
+    if (
+      current.route.element &&
+      (
+        current.route.element as React.ReactElement<any, React.NamedExoticComponent>
+      ).type.displayName?.endsWith('Redirect')
+    ) {
+      // Redirect routes are not relevant to us.
+      continue;
+    }
+
+    // We are on a terminal route in the tree. Add to the map of route components.
+    // We are less interested in container route components.
+    if (current.route.element) {
+      routes.add(currentPath);
     }
   }
 
-  return routeMap;
+  return routes;
 }
 
 describe('buildRoutes()', function () {
@@ -83,16 +75,16 @@ describe('buildRoutes()', function () {
 
     // Get routes for with customer domains off.
     spy.mockReturnValue(false);
-    const routeMap = extractRoutes(buildRoutes());
+    const routes = extractRoutes(buildReactRouter6Routes(buildRoutes()));
 
     // Get routes with customer domains on.
     spy.mockReturnValue(true);
-    const domainRoutes = extractRoutes(buildRoutes());
+    const domainRoutes = extractRoutes(buildReactRouter6Routes(buildRoutes()));
 
     // All routes that exist under orgId path slugs should
     // have a sibling under customer-domains.
     const mismatch: Array<{domain: string; slug: string}> = [];
-    for (const path in routeMap) {
+    for (const path of routes) {
       // Normalize the URLs so that we know the path we're looking for.
       const domainPath = normalizeUrl(path, {forceCustomerDomain: true});
 
@@ -101,7 +93,7 @@ describe('buildRoutes()', function () {
         continue;
       }
 
-      if (!domainRoutes[domainPath]) {
+      if (!domainRoutes.has(domainPath)) {
         mismatch.push({slug: path, domain: domainPath});
       }
     }

+ 1 - 3
static/app/routes.tsx

@@ -1,6 +1,4 @@
 import {Fragment, lazy} from 'react';
-// biome-ignore lint/nursery/noRestrictedImports: warning
-import {IndexRedirect, Redirect} from 'react-router';
 import memoize from 'lodash/memoize';
 
 import LazyLoad from 'sentry/components/lazyLoad';
@@ -32,7 +30,7 @@ import redirectDeprecatedProjectRoute from 'sentry/views/projects/redirectDeprec
 import RouteNotFound from 'sentry/views/routeNotFound';
 import SettingsWrapper from 'sentry/views/settings/components/settingsWrapper';
 
-import {IndexRoute, Route} from './components/route';
+import {IndexRedirect, IndexRoute, Redirect, Route} from './components/route';
 
 const hook = (name: HookName) => HookStore.get(name).map(cb => cb());
 

+ 9 - 0
static/app/types/legacyReactRouter.tsx

@@ -130,3 +130,12 @@ export interface RouteContextInterface<P = Record<string, string>, Q = any> {
 }
 
 export type Route = React.ComponentClass<RouteProps>;
+
+export interface IndexRedirectProps {
+  to: RoutePattern;
+  query?: Query | undefined;
+}
+
+export interface RedirectProps extends IndexRedirectProps {
+  from: RoutePattern;
+}