From 2d6e1f66f10e2fca95a503cf56db3feb78bb746d Mon Sep 17 00:00:00 2001 From: Alex Ilyaev Date: Mon, 17 Jul 2017 16:13:44 +0300 Subject: [PATCH] Redirect: Support props changes on Redirect to handle edge cases --- packages/react-router/modules/Redirect.js | 24 ++++- .../modules/__tests__/Switch-test.js | 100 ++++++++++++++++++ 2 files changed, 122 insertions(+), 2 deletions(-) diff --git a/packages/react-router/modules/Redirect.js b/packages/react-router/modules/Redirect.js index 87fe342789..ee7fe44f51 100644 --- a/packages/react-router/modules/Redirect.js +++ b/packages/react-router/modules/Redirect.js @@ -1,9 +1,11 @@ import React from 'react' import PropTypes from 'prop-types' +import warning from 'warning' import invariant from 'invariant' +import { locationsAreEqual } from 'history' /** - * The public API for updating the location programatically + * The public API for updating the location programmatically * with a component. */ class Redirect extends React.Component { @@ -13,7 +15,7 @@ class Redirect extends React.Component { to: PropTypes.oneOfType([ PropTypes.string, PropTypes.object - ]) + ]).isRequired } static defaultProps = { @@ -49,6 +51,24 @@ class Redirect extends React.Component { this.perform() } + componentDidUpdate(prevProps) { + let prevTo = prevProps.to + let nextTo = this.props.to + + prevTo = typeof prevTo === 'string' ? { pathname: prevTo } : prevTo + nextTo = typeof nextTo === 'string' ? { pathname: nextTo } : nextTo + + if (locationsAreEqual(prevTo, nextTo)) + return warning( + false, + `You tried to redirect to the same route you're currently on: "%s%s"`, + nextTo.pathname, + nextTo.search || '' + ) + + this.perform() + } + perform() { const { history } = this.context.router const { push, to } = this.props diff --git a/packages/react-router/modules/__tests__/Switch-test.js b/packages/react-router/modules/__tests__/Switch-test.js index 76c006a24a..3a8ce54c63 100644 --- a/packages/react-router/modules/__tests__/Switch-test.js +++ b/packages/react-router/modules/__tests__/Switch-test.js @@ -102,6 +102,106 @@ describe('A ', () => { expect(node.innerHTML).toContain('bub') }) + it('handles subsequent redirects', () => { + const node = document.createElement('div') + + ReactDOM.render(( + + + + + +
three
}/> +
+
+ ), node) + + expect(node.innerHTML).toContain('three') + }) + + it('warns when redirecting to same route, both strings', () => { + const node = document.createElement('div') + let redirected = false + + expect.spyOn(console, 'error') + + ReactDOM.render(( + + + { + if (!redirected) { + return + } + + return + }}/> + + + ), node) + + expect(console.error.calls.length).toBe(1) + expect(console.error.calls[0].arguments[0]).toMatch(/Warning:.*"\/one"/) + expect.restoreSpies() + }) + + it('warns when redirecting to same route, mixed types', () => { + const node = document.createElement('div') + let redirected = false + + expect.spyOn(console, 'error') + + ReactDOM.render(( + + + { + if (!redirected) { + redirected = true + return + } + + return + }}/> + + + ), node) + + expect(console.error.calls.length).toBe(1) + expect(console.error.calls[0].arguments[0]).toMatch(/Warning:.*"\/one"/) + expect.restoreSpies() + }) + + it('does NOT warn when redirecting to same route with different `search`', () => { + const node = document.createElement('div') + let redirected = false + let done = false + + expect.spyOn(console, 'error') + + ReactDOM.render(( + + + { + if (done) + return

done

+ + if (!redirected) { + redirected = true + return + } + + done = true + + return + }}/> +
+
+ ), node) + + expect(node.innerHTML).toContain('done') + expect(console.error.calls.length).toBe(0) + expect.restoreSpies() + }) + it('handles comments', () => { const node = document.createElement('div')