Treat all navigation as a side effect

- Removes <Redirect> - Removes support for { redirectTo } in useRoutes() - Warns if navigate() is used during the initial render - Warns when <Navigate> is used on the initial render in <StaticRouter> This began as a discussion on GitHub (see #7203). Essentially we need to treat all navigation as a side effect, which means it needs to happen in a useEffect. If people need v5's <Redirect> functionality (when it was used in the route config inside a <Switch>), they either need to a) do the redirect on the server (the best solution) or b) render a <Navigate> on the initial render. However, (b) will only show the new route on the subsequent render. This should also eliminate warnings in the dev-experimental builds. Closes #7203 Fixes #7199
remix-run · Apr 3, 2020 · cbcd398 · cbcd398
1 parent f59ee54
commit cbcd398
Show file tree

Hide file tree

Showing 15 changed files with 137 additions and 408 deletions.
diff --git a/docs/advanced-guides/README.md b/docs/advanced-guides/README.md
@@ -8,7 +8,6 @@ build a certain capability or feature of your app with React Router.
 - [Using Layouts](#TODO)
 - ["Not Found" Routes (404)](#TODO)
 - [Using Route Config Objects](#TODO)
-- [Using Redirects](#TODO)
 
 ## Common User Flows
 
@@ -31,7 +30,6 @@ build a certain capability or feature of your app with React Router.
 
 - [Creating Routes from Files](#TODO)
 - [Using `StaticRouter` Directly](#TODO)
-- [Handling Redirects](#TODO)
 
 ## Testing
 

diff --git a/docs/advanced-guides/migrating-5-to-6.md b/docs/advanced-guides/migrating-5-to-6.md
@@ -36,8 +36,7 @@ In general, the process looks like this:
     - Consolidate your `<Route>`s into a nested config (optional)
   - [Use `navigate` instead of `history`](#use-navigate-instead-of-history)
     - Use `useNavigate` hook instead of `useHistory`
-    - Use `<Navigate>` instead of `<Redirect>` (outside of route configs)
-      - No need to change `<Redirect>` directly inside `<Routes>`
+    - Use `<Navigate>` instead of `<Redirect>`
   - [Use `useRoutes` instead of `react-router-config`](#use-useroutes-instead-of-react-router-config)
   - [Rename `<Link component>` to `<Link as>`](#rename-link-component-to-link-as)
   - [Get `StaticRouter` from `react-router-dom/server`](#get-staticrouter-from-react-router-domserver)
@@ -521,8 +520,6 @@ function App() {
         { path: 'sent', element: <SentInvoices /> }
       ]
     },
-    // Redirects use a redirectTo property to
-    { path: 'home', redirectTo: '/' },
     // Not found routes work as you'd expect
     { path: '*', element: <NotFound /> }
   ]);
@@ -657,9 +654,12 @@ The `navigate` API is aware of the internal pending transition state and will
 do a REPLACE instead of a PUSH onto the history stack, so the user doesn't end
 up with pages in their history that never actually loaded.
 
-*Note: You should still use a `<Redirect>` as part of your route config
-(inside a `<Routes>`). This change is only necessary for `<Redirect>`s that are
-used to navigate in response to user interaction.*
+*Note: The `<Redirect>` element from v5 is no longer supported as part of your
+route config (inside a `<Routes>`). This is due to upcoming changes in React
+that make it unsafe to alter the state of the router during the initial render.
+If you need to redirect immediately, you can either a) do it on your server
+(probably the best solution) or b) render a `<Navigate>` element in your route
+component. However, recognize that the navigation will happen in a `useEffect`.*
 
 Aside from suspense compatibility, `navigate`, like `Link`, supports relative
 navigation. For example:

diff --git a/docs/advanced-guides/testing/testing-with-mocha.md b/docs/advanced-guides/testing/testing-with-mocha.md
@@ -4,7 +4,7 @@
 
 TODO
 
-## Testing Routes and Redirects
+## Testing Routes
 
 TODO
 

diff --git a/docs/advanced-guides/testing/testing-with-react-test-renderer.md b/docs/advanced-guides/testing/testing-with-react-test-renderer.md
@@ -4,7 +4,7 @@
 
 TODO
 
-## Testing Routes and Redirects
+## Testing Routes
 
 TODO
 

diff --git a/docs/advanced-guides/testing/testing-with-react-testing-library.md b/docs/advanced-guides/testing/testing-with-react-testing-library.md
@@ -4,7 +4,7 @@
 
 TODO
 
-## Testing Routes and Redirects
+## Testing Routes
 
 TODO
 

diff --git a/docs/main-concepts.md b/docs/main-concepts.md
@@ -15,7 +15,6 @@ We will cover the following topics:
   - [Params and Search (Query) Strings](#params-and-search-query-strings)
 - [Routing and Navigation](#routing-and-navigation)
   - [Layouts and Nesting](#layouts-and-nesting)
-  - [Redirects](#redirects)
   - [Transitions](#transitions)
   - [Prompts](#prompts)
 
@@ -39,10 +38,6 @@ TODO
 
 TODO
 
-### Redirects
-
-TODO
-
 ### Transitions
 
 TODO

diff --git a/docs/tutorial/README.md b/docs/tutorial/README.md
@@ -22,5 +22,5 @@ function ProductLayout() {
 ```
 
 - Links
-- Redirects (main uses are for preserving old URLs and doing auth)
+- Redirects (after auth)
 - Descendant `<Routes>` (rendered somewhere further down the component tree)
diff --git a/packages/react-router-dom/__tests__/navigate-encode-params-test.js b/packages/react-router-dom/__tests__/navigate-encode-params-test.js
@@ -25,7 +25,11 @@ describe('navigate with params', () => {
     it('correctly encodes the param in the URL and decodes the param when it is used', () => {
       function Start() {
         let navigate = useNavigate();
-        navigate('/blog/react router');
+
+        React.useEffect(() => {
+          navigate('/blog/react router');
+        });
+
         return null;
       }
 
@@ -55,7 +59,11 @@ describe('navigate with params', () => {
     it('does not alter the param encoding in the URL and decodes the param when it is used', () => {
       function Start() {
         let navigate = useNavigate();
-        navigate('/blog/react+router');
+
+        React.useEffect(() => {
+          navigate('/blog/react+router');
+        });
+
         return null;
       }
 

diff --git a/packages/react-router-dom/__tests__/static-navigate-test.js b/packages/react-router-dom/__tests__/static-navigate-test.js
@@ -4,178 +4,30 @@ import { Navigate, Routes, Route } from 'react-router-dom';
 import { StaticRouter as Router } from 'react-router-dom/server';
 
 describe('A <Navigate> in a <StaticRouter>', () => {
-  describe('a replace navigation', () => {
-    it('mutates the context object', () => {
-      let context = {};
-
-      function Home() {
-        return <Navigate replace to="/somewhere-else?the=query" />;
-      }
-
-      ReactDOMServer.renderToStaticMarkup(
-        <Router context={context} location="/home">
-          <Routes>
-            <Route path="/home" element={<Home />} />
-          </Routes>
-        </Router>
-      );
-
-      expect(context).toMatchObject({
-        url: '/somewhere-else?the=query',
-        state: undefined
-      });
-    });
-
-    describe('with an object to prop', () => {
-      it('mutates the context object', () => {
-        let context = {};
-
-        function Home() {
-          return (
-            <Navigate
-              replace
-              to={{ pathname: '/somewhere-else', search: '?the=query' }}
-            />
-          );
-        }
-
-        ReactDOMServer.renderToStaticMarkup(
-          <Router context={context} location="/home">
-            <Routes>
-              <Route path="/home" element={<Home />} />
-            </Routes>
-          </Router>
-        );
-
-        expect(context).toMatchObject({
-          url: '/somewhere-else?the=query',
-          state: undefined
-        });
-      });
-
-      it('reuses pieces of the current location that are missing from the to value', () => {
-        let context = {};
-
-        function Home() {
-          // only use search here, so pathname should be reused
-          return <Navigate replace to={{ search: '?the=query' }} />;
-        }
-
-        ReactDOMServer.renderToStaticMarkup(
-          <Router context={context} location="/home">
-            <Routes>
-              <Route path="/home" element={<Home />} />
-            </Routes>
-          </Router>
-        );
-
-        expect(context).toMatchObject({
-          url: '/home?the=query',
-          state: undefined
-        });
-      });
-    });
+  let consoleWarn;
+  beforeEach(() => {
+    consoleWarn = jest.spyOn(console, 'warn').mockImplementation(() => {});
   });
 
-  describe('a push navigation', () => {
-    let consoleWarn;
-    beforeEach(() => {
-      consoleWarn = jest.spyOn(console, 'warn').mockImplementation(() => {});
-    });
-
-    afterEach(() => {
-      consoleWarn.mockRestore();
-    });
-
-    it('issues a warning', () => {
-      let context = {};
-
-      function Home() {
-        return <Navigate push to="/somewhere-else?the=query" />;
-      }
-
-      ReactDOMServer.renderToStaticMarkup(
-        <Router context={context} location="/home">
-          <Routes>
-            <Route path="/home" element={<Home />} />
-          </Routes>
-        </Router>
-      );
-
-      expect(consoleWarn).toHaveBeenCalledWith(
-        expect.stringContaining('cannot perform a PUSH with a <StaticRouter>')
-      );
-    });
-
-    it('mutates the context object', () => {
-      let context = {};
-
-      function Home() {
-        return <Navigate push to="/somewhere-else?the=query" />;
-      }
-
-      ReactDOMServer.renderToStaticMarkup(
-        <Router context={context} location="/home">
-          <Routes>
-            <Route path="/home" element={<Home />} />
-          </Routes>
-        </Router>
-      );
-
-      expect(context).toMatchObject({
-        url: '/somewhere-else?the=query',
-        state: undefined
-      });
-    });
-
-    describe('with an object to prop', () => {
-      it('mutates the context object', () => {
-        let context = {};
-
-        function Home() {
-          return (
-            <Navigate
-              push
-              to={{ pathname: '/somewhere-else', search: '?the=query' }}
-            />
-          );
-        }
-
-        ReactDOMServer.renderToStaticMarkup(
-          <Router context={context} location="/home">
-            <Routes>
-              <Route path="/home" element={<Home />} />
-            </Routes>
-          </Router>
-        );
-
-        expect(context).toMatchObject({
-          url: '/somewhere-else?the=query',
-          state: undefined
-        });
-      });
-
-      it('reuses pieces of the current location that are missing from the to value', () => {
-        let context = {};
-
-        function Home() {
-          // only use search here, so pathname should be reused
-          return <Navigate push to={{ search: '?the=query' }} />;
-        }
-
-        ReactDOMServer.renderToStaticMarkup(
-          <Router context={context} location="/home">
-            <Routes>
-              <Route path="/home" element={<Home />} />
-            </Routes>
-          </Router>
-        );
+  afterEach(() => {
+    consoleWarn.mockRestore();
+  });
 
-        expect(context).toMatchObject({
-          url: '/home?the=query',
-          state: undefined
-        });
-      });
-    });
+  it('warns about using on the initial render', () => {
+    function Home() {
+      return <Navigate to="/somewhere-else?the=query" />;
+    }
+
+    ReactDOMServer.renderToStaticMarkup(
+      <Router location="/home">
+        <Routes>
+          <Route path="/home" element={<Home />} />
+        </Routes>
+      </Router>
+    );
+
+    expect(consoleWarn).toHaveBeenCalledWith(
+      expect.stringMatching('not be rendered on the initial render')
+    );
   });
 });
diff --git a/packages/react-router-dom/__tests__/static-redirect-test.js b/packages/react-router-dom/__tests__/static-redirect-test.js
diff --git a/packages/react-router-dom/index.js b/packages/react-router-dom/index.js
@@ -6,7 +6,6 @@ import {
   MemoryRouter,
   Navigate,
   Outlet,
-  Redirect,
   Route,
   Router,
   Routes,
@@ -49,7 +48,6 @@ export {
   MemoryRouter,
   Navigate,
   Outlet,
-  Redirect,
   Route,
   Router,
   Routes,