From 2d6e1f66f10e2fca95a503cf56db3feb78bb746d Mon Sep 17 00:00:00 2001 From: Alex Ilyaev Date: Mon, 17 Jul 2017 16:13:44 +0300 Subject: [PATCH 1/3] 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') From a3a03d42f894670575b7da5760a7c279e5764a74 Mon Sep 17 00:00:00 2001 From: Alex Ilyaev Date: Mon, 17 Jul 2017 19:21:57 +0300 Subject: [PATCH 2/3] Redirect: Fixed tests after moving to Jest --- .../modules/__tests__/Switch-test.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/packages/react-router/modules/__tests__/Switch-test.js b/packages/react-router/modules/__tests__/Switch-test.js index 3a8ce54c63..41eb72302d 100644 --- a/packages/react-router/modules/__tests__/Switch-test.js +++ b/packages/react-router/modules/__tests__/Switch-test.js @@ -123,7 +123,7 @@ describe('A ', () => { const node = document.createElement('div') let redirected = false - expect.spyOn(console, 'error') + spyOn(console, 'error') ReactDOM.render(( @@ -139,16 +139,15 @@ describe('A ', () => { ), node) - expect(console.error.calls.length).toBe(1) - expect(console.error.calls[0].arguments[0]).toMatch(/Warning:.*"\/one"/) - expect.restoreSpies() + expect(console.error.calls.count()).toBe(1) + expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one"/) }) it('warns when redirecting to same route, mixed types', () => { const node = document.createElement('div') let redirected = false - expect.spyOn(console, 'error') + spyOn(console, 'error') ReactDOM.render(( @@ -165,9 +164,8 @@ describe('A ', () => { ), node) - expect(console.error.calls.length).toBe(1) - expect(console.error.calls[0].arguments[0]).toMatch(/Warning:.*"\/one"/) - expect.restoreSpies() + expect(console.error.calls.count()).toBe(1) + expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one"/) }) it('does NOT warn when redirecting to same route with different `search`', () => { @@ -175,7 +173,7 @@ describe('A ', () => { let redirected = false let done = false - expect.spyOn(console, 'error') + spyOn(console, 'error') ReactDOM.render(( @@ -198,8 +196,7 @@ describe('A ', () => { ), node) expect(node.innerHTML).toContain('done') - expect(console.error.calls.length).toBe(0) - expect.restoreSpies() + expect(console.error.calls.count()).toBe(0) }) it('handles comments', () => { From b1a67c6a3e6e25e4804f8517f02e28ed1e9a7996 Mon Sep 17 00:00:00 2001 From: Alex Ilyaev Date: Wed, 23 Aug 2017 02:53:49 +0300 Subject: [PATCH 3/3] Use `createLocation` to parse `to` to properly handle edge cases --- packages/react-router/modules/Redirect.js | 21 +++----- .../modules/__tests__/Switch-test.js | 50 +++++++++++++++++-- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/packages/react-router/modules/Redirect.js b/packages/react-router/modules/Redirect.js index ee7fe44f51..3dc43a5130 100644 --- a/packages/react-router/modules/Redirect.js +++ b/packages/react-router/modules/Redirect.js @@ -2,7 +2,7 @@ import React from 'react' import PropTypes from 'prop-types' import warning from 'warning' import invariant from 'invariant' -import { locationsAreEqual } from 'history' +import { createLocation, locationsAreEqual } from 'history' /** * The public API for updating the location programmatically @@ -52,19 +52,14 @@ class Redirect extends React.Component { } componentDidUpdate(prevProps) { - let prevTo = prevProps.to - let nextTo = this.props.to + const prevTo = createLocation(prevProps.to) + const nextTo = createLocation(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 || '' - ) + if (locationsAreEqual(prevTo, nextTo)) { + warning(false, `You tried to redirect to the same route you're currently on: ` + + `"${nextTo.pathname}${nextTo.search}"`) + return + } this.perform() } diff --git a/packages/react-router/modules/__tests__/Switch-test.js b/packages/react-router/modules/__tests__/Switch-test.js index 41eb72302d..b672d84f0d 100644 --- a/packages/react-router/modules/__tests__/Switch-test.js +++ b/packages/react-router/modules/__tests__/Switch-test.js @@ -122,6 +122,7 @@ describe('A ', () => { it('warns when redirecting to same route, both strings', () => { const node = document.createElement('div') let redirected = false + let done = false spyOn(console, 'error') @@ -129,9 +130,13 @@ describe('A ', () => { { + if (done) + return

done

+ if (!redirected) { return } + done = true return }}/> @@ -139,6 +144,7 @@ describe('A ', () => { ), node) + expect(node.innerHTML).not.toContain('done') expect(console.error.calls.count()).toBe(1) expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one"/) }) @@ -146,6 +152,7 @@ describe('A ', () => { it('warns when redirecting to same route, mixed types', () => { const node = document.createElement('div') let redirected = false + let done = false spyOn(console, 'error') @@ -153,22 +160,27 @@ describe('A ', () => { { + if (done) + return

done

+ if (!redirected) { redirected = true return } + done = true - return + return }}/> ), node) + expect(node.innerHTML).not.toContain('done') expect(console.error.calls.count()).toBe(1) expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one"/) }) - it('does NOT warn when redirecting to same route with different `search`', () => { + it('warns when redirecting to same route, mixed types, string with query', () => { const node = document.createElement('div') let redirected = false let done = false @@ -184,12 +196,42 @@ describe('A ', () => { if (!redirected) { redirected = true - return + return } + done = true + return + }}/> + + + ), node) + + expect(node.innerHTML).not.toContain('done') + expect(console.error.calls.count()).toBe(1) + expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one\?utm=1"/) + }) + + it('does NOT warn when redirecting to same route with different `search`', () => { + const node = document.createElement('div') + let redirected = false + let done = false + + spyOn(console, 'error') + + ReactDOM.render(( + + + { + if (done) + return

done

+ + if (!redirected) { + redirected = true + return + } done = true - return + return }}/>