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 inadvertent re-renders when using Component instead of element #10287

Merged
merged 7 commits into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/fix-component-rerenders-router.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Deprecate the `createRouter` `detectErrorBoundary` option in favor of the new `mapRouteProperties` option for converting a framework-agnostic route to a framework-aware route. This allows us to set more than just the `hasErrorBoundary` property during route pre-processing, and is now used for mapping `Component -> element` and `ErrorBoundary -> errorElement` in `react-router`.
6 changes: 6 additions & 0 deletions .changeset/fix-component-rerenders.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"react-router": patch
"react-router-dom": patch
---

Fix inadvertent re-renders when using `Component` instead of `element` on a route definition
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,13 @@
},
"filesize": {
"packages/router/dist/router.umd.min.js": {
"none": "44 kB"
"none": "44.1 kB"
},
"packages/react-router/dist/react-router.production.min.js": {
"none": "13 kB"
"none": "13.1 kB"
},
"packages/react-router/dist/umd/react-router.production.min.js": {
"none": "15.1 kB"
"none": "15.3 kB"
},
"packages/react-router-dom/dist/react-router-dom.production.min.js": {
"none": "11.6 kB"
Expand Down
15 changes: 10 additions & 5 deletions packages/react-router-dom/__tests__/data-static-router-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -992,14 +992,15 @@ describe("A <StaticRouterProvider>", () => {
expect(router.routes).toMatchInlineSnapshot(`
[
{
"ErrorBoundary": [Function],
"ErrorBoundary": undefined,
"children": [
{
"ErrorBoundary": [Function],
"ErrorBoundary": undefined,
"children": undefined,
"element": <h2>
Hi again!
</h2>,
"errorElement": <ErrorBoundary />,
"hasErrorBoundary": true,
"id": "0-0",
"path": "path",
Expand All @@ -1008,6 +1009,7 @@ describe("A <StaticRouterProvider>", () => {
"element": <h1>
Hi!
</h1>,
"errorElement": <ErrorBoundary />,
"hasErrorBoundary": true,
"id": "0",
"path": "the",
Expand All @@ -1021,14 +1023,15 @@ describe("A <StaticRouterProvider>", () => {
"pathname": "/the",
"pathnameBase": "/the",
"route": {
"ErrorBoundary": [Function],
"ErrorBoundary": undefined,
"children": [
{
"ErrorBoundary": [Function],
"ErrorBoundary": undefined,
"children": undefined,
"element": <h2>
Hi again!
</h2>,
"errorElement": <ErrorBoundary />,
"hasErrorBoundary": true,
"id": "0-0",
"path": "path",
Expand All @@ -1037,6 +1040,7 @@ describe("A <StaticRouterProvider>", () => {
"element": <h1>
Hi!
</h1>,
"errorElement": <ErrorBoundary />,
"hasErrorBoundary": true,
"id": "0",
"path": "the",
Expand All @@ -1047,11 +1051,12 @@ describe("A <StaticRouterProvider>", () => {
"pathname": "/the/path",
"pathnameBase": "/the/path",
"route": {
"ErrorBoundary": [Function],
"ErrorBoundary": undefined,
"children": undefined,
"element": <h2>
Hi again!
</h2>,
"errorElement": <ErrorBoundary />,
"hasErrorBoundary": true,
"id": "0-0",
"path": "path",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-router-dom/__tests__/exports-test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as ReactRouter from "react-router";
import * as ReactRouterDOM from "react-router-dom";

let nonReExportedKeys = new Set(["UNSAFE_detectErrorBoundary"]);
let nonReExportedKeys = new Set(["UNSAFE_mapRouteProperties"]);

describe("react-router-dom", () => {
for (let key in ReactRouter) {
Expand Down
6 changes: 3 additions & 3 deletions packages/react-router-dom/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
UNSAFE_DataRouterStateContext as DataRouterStateContext,
UNSAFE_NavigationContext as NavigationContext,
UNSAFE_RouteContext as RouteContext,
UNSAFE_detectErrorBoundary as detectErrorBoundary,
UNSAFE_mapRouteProperties as mapRouteProperties,
} from "react-router";
import type {
BrowserHistory,
Expand Down Expand Up @@ -220,7 +220,7 @@ export function createBrowserRouter(
history: createBrowserHistory({ window: opts?.window }),
hydrationData: opts?.hydrationData || parseHydrationData(),
routes,
detectErrorBoundary,
mapRouteProperties,
}).initialize();
}

Expand All @@ -234,7 +234,7 @@ export function createHashRouter(
history: createHashHistory({ window: opts?.window }),
hydrationData: opts?.hydrationData || parseHydrationData(),
routes,
detectErrorBoundary,
mapRouteProperties,
}).initialize();
}

Expand Down
10 changes: 4 additions & 6 deletions packages/react-router-dom/server.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
createStaticHandler as routerCreateStaticHandler,
UNSAFE_convertRoutesToDataRoutes as convertRoutesToDataRoutes,
} from "@remix-run/router";
import { UNSAFE_mapRouteProperties as mapRouteProperties } from "react-router";
import type { Location, RouteObject, To } from "react-router-dom";
import { Routes } from "react-router-dom";
import {
Expand Down Expand Up @@ -206,12 +207,9 @@ function getStatelessNavigator() {
};
}

let detectErrorBoundary = (route: RouteObject) =>
Boolean(route.ErrorBoundary) || Boolean(route.errorElement);

type CreateStaticHandlerOptions = Omit<
RouterCreateStaticHandlerOptions,
"detectErrorBoundary"
"detectErrorBoundary" | "mapRouteProperties"
>;

export function createStaticHandler(
Expand All @@ -220,7 +218,7 @@ export function createStaticHandler(
) {
return routerCreateStaticHandler(routes, {
...opts,
detectErrorBoundary,
mapRouteProperties,
});
}

Expand All @@ -231,7 +229,7 @@ export function createStaticRouter(
let manifest: RouteManifest = {};
let dataRoutes = convertRoutesToDataRoutes(
routes,
detectErrorBoundary,
mapRouteProperties,
undefined,
manifest
);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-router-native/__tests__/exports-test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as ReactRouter from "react-router";
import * as ReactRouterNative from "react-router-native";

let nonReExportedKeys = new Set(["UNSAFE_detectErrorBoundary"]);
let nonReExportedKeys = new Set(["UNSAFE_mapRouteProperties"]);

describe("react-router-native", () => {
for (let key in ReactRouter) {
Expand Down
58 changes: 39 additions & 19 deletions packages/react-router/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as React from "react";
import type {
ActionFunction,
ActionFunctionArgs,
Expand Down Expand Up @@ -207,27 +208,46 @@ export {
useRoutes,
};

function detectErrorBoundary(route: RouteObject) {
if (__DEV__) {
if (route.Component && route.element) {
warning(
false,
"You should not include both `Component` and `element` on your route - " +
"`element` will be ignored."
);
function mapRouteProperties(route: RouteObject) {
let updates: Partial<RouteObject> & { hasErrorBoundary: boolean } = {
// Note: this check also occurs in createRoutesFromChildren so update
// there if you change this -- please and thank you!
hasErrorBoundary: route.ErrorBoundary != null || route.errorElement != null,
};

if (route.Component) {
if (__DEV__) {
if (route.element) {
warning(
false,
"You should not include both `Component` and `element` on your route - " +
"`Component` will be used."
);
}
}
if (route.ErrorBoundary && route.errorElement) {
warning(
false,
"You should not include both `ErrorBoundary` and `errorElement` on your route - " +
"`errorElement` will be ignored."
);
Object.assign(updates, {
element: React.createElement(route.Component),
Component: undefined,
});
}

if (route.ErrorBoundary) {
if (__DEV__) {
if (route.errorElement) {
warning(
false,
"You should not include both `ErrorBoundary` and `errorElement` on your route - " +
"`ErrorBoundary` will be used."
);
}
}
Object.assign(updates, {
errorElement: React.createElement(route.ErrorBoundary),
ErrorBoundary: undefined,
});
}

// Note: this check also occurs in createRoutesFromChildren so update
// there if you change this
return Boolean(route.ErrorBoundary) || Boolean(route.errorElement);
return updates;
}

export function createMemoryRouter(
Expand All @@ -249,7 +269,7 @@ export function createMemoryRouter(
}),
hydrationData: opts?.hydrationData,
routes,
detectErrorBoundary,
mapRouteProperties,
}).initialize();
}

Expand All @@ -273,5 +293,5 @@ export {
RouteContext as UNSAFE_RouteContext,
DataRouterContext as UNSAFE_DataRouterContext,
DataRouterStateContext as UNSAFE_DataRouterStateContext,
detectErrorBoundary as UNSAFE_detectErrorBoundary,
mapRouteProperties as UNSAFE_mapRouteProperties,
};
16 changes: 6 additions & 10 deletions packages/react-router/lib/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,8 @@ function DefaultErrorComponent() {
);
}

const defaultErrorElement = <DefaultErrorComponent />;

type RenderErrorBoundaryProps = React.PropsWithChildren<{
location: Location;
error: any;
Expand Down Expand Up @@ -635,23 +637,17 @@ export function _renderMatches(
// Only data routers handle errors
let errorElement: React.ReactNode | null = null;
if (dataRouterState) {
if (match.route.ErrorBoundary) {
errorElement = <match.route.ErrorBoundary />;
} else if (match.route.errorElement) {
errorElement = match.route.errorElement;
} else {
errorElement = <DefaultErrorComponent />;
}
errorElement = match.route.errorElement || defaultErrorElement;
}
let matches = parentMatches.concat(renderedMatches.slice(0, index + 1));
let getChildren = () => {
let children: React.ReactNode = outlet;
let children: React.ReactNode;
if (error) {
children = errorElement;
} else if (match.route.Component) {
children = <match.route.Component />;
} else if (match.route.element) {
children = match.route.element;
} else {
children = outlet;
}
return (
<RenderedRoute
Expand Down
17 changes: 11 additions & 6 deletions packages/router/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

The `@remix-run/router` package is a framework-agnostic routing package (sometimes referred to as a browser-emulator) that serves as the heart of [React Router][react-router] and [Remix][remix] and provides all the core functionality for routing coupled with data loading and data mutations. It comes with built-in handling of errors, race-conditions, interruptions, cancellations, lazy-loading data, and much, much more.

If you're using React Router, you should never `import` anything directly from the `@remix-run/router` or `react-router` packages, but you should have everything you need in either `react-router-dom` or `react-router-native`. Both of those packages re-export everything from `@remix-run/router` and `react-router`.
If you're using React Router, you should never `import` anything directly from the `@remix-run/router` - you should have everything you need in `react-router-dom` (or `react-router`/`react-router-native` if you're not rendering in the browser). All of those packages should re-export everything you would otherwise need from `@remix-run/router`.

> **Warning**
>
Expand All @@ -16,11 +16,16 @@ A Router instance can be created using `createRouter`:
// Create and initialize a router. "initialize" contains all side effects
// including history listeners and kicking off the initial data fetch
let router = createRouter({
// Routes array
routes: ,
// History instance
history,
}).initialize()
// Required properties
routes, // Routes array
history, // History instance

// Optional properties
basename, // Base path
mapRouteProperties, // Map function framework-agnostic routes to framework-aware routes
future, // Future flags
hydrationData, // Hydration data if using server-side-rendering
}).initialize();
```

Internally, the Router represents the state in an object of the following format, which is available through `router.state`. You can also register a subscriber of the signature `(state: RouterState) => void` to execute when the state updates via `router.subscribe()`;
Expand Down
Loading