From b210ae5bb4e508ad939ccdcbcbb80a38ac1e8885 Mon Sep 17 00:00:00 2001 From: Benoit Tremblay Date: Sun, 9 Apr 2017 13:00:40 -0400 Subject: [PATCH 1/6] Explicit error on outside When using outside a valid , the message is not that explicit. It throws: `Cannot read property 'route' of undefined` Even though it's not that bad, I propose we make it even more explicit. Can't be bad to know where the error is coming from. If you made a typo when importing the router, it's not obvious otherwise. This will throw an error rather than a warning because the library code would crash otherwise. --- packages/react-router/modules/Route.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/react-router/modules/Route.js b/packages/react-router/modules/Route.js index 8838750285..1d9bc715cd 100644 --- a/packages/react-router/modules/Route.js +++ b/packages/react-router/modules/Route.js @@ -33,6 +33,10 @@ class Route extends React.Component { } getChildContext() { + if (!this.context.router) { + throw new Error('You should not use or withRoute() outside a valid '); + } + return { router: { ...this.context.router, From bb378902581244ce4d8f23e4bc9fdd1ddb722158 Mon Sep 17 00:00:00 2001 From: Benoit Tremblay Date: Sun, 9 Apr 2017 13:12:27 -0400 Subject: [PATCH 2/6] Add unit test --- packages/react-router/modules/Route.js | 8 +++---- .../modules/__tests__/Route-test.js | 23 ++++++++----------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/packages/react-router/modules/Route.js b/packages/react-router/modules/Route.js index 1d9bc715cd..f6358cba85 100644 --- a/packages/react-router/modules/Route.js +++ b/packages/react-router/modules/Route.js @@ -34,7 +34,7 @@ class Route extends React.Component { getChildContext() { if (!this.context.router) { - throw new Error('You should not use or withRoute() outside a valid '); + throw new Error('You should not use or withRoute() outside a valid ') } return { @@ -66,17 +66,17 @@ class Route extends React.Component { warning( !(component && render), - 'You should not use and in the same route; will be ignored' + 'You should not use and in the same route; will be ignored' ) warning( !(component && children), - 'You should not use and in the same route; will be ignored' + 'You should not use and in the same route; will be ignored' ) warning( !(render && children), - 'You should not use and in the same route; will be ignored' + 'You should not use and in the same route; will be ignored' ) } diff --git a/packages/react-router/modules/__tests__/Route-test.js b/packages/react-router/modules/__tests__/Route-test.js index 17744980cb..5e810239b4 100644 --- a/packages/react-router/modules/__tests__/Route-test.js +++ b/packages/react-router/modules/__tests__/Route-test.js @@ -57,20 +57,13 @@ describe('A ', () => { }) it('supports preact by nulling out children prop when empty array is passed', () => { - const TEXT = 'Mrs. Kato' const node = document.createElement('div') - ReactDOM.render(( - - ( -

{TEXT}

- )}> - {[]} -
-
- ), node) - - expect(node.innerHTML).toContain(TEXT) + expect(() => { + ReactDOM.render(( + null} /> + ), node) + }).toThrow(/You should not use or withRoute\(\) outside a valid /) }) it('matches using nextContext when updating', () => { @@ -88,6 +81,10 @@ describe('A ', () => { push('/sushi/spicy-tuna') expect(node.innerHTML).toContain('/sushi/spicy-tuna') }) + + it('crash explicitly with no valid ', () => { + + }) }) describe('A with dynamic segments in the path', () => { @@ -342,7 +339,7 @@ describe('A ', () => { expect(node.innerHTML).toContain(TEXT) }) - + it('continues to use parent\'s prop location after navigation', () => { const TEXT = 'cheddar pretzel' const node = document.createElement('div') From 2f38409bbb353faee39d9ff035dbe24fcbdad426 Mon Sep 17 00:00:00 2001 From: Benoit Tremblay Date: Sun, 9 Apr 2017 13:14:56 -0400 Subject: [PATCH 3/6] Fix unit test change --- .../modules/__tests__/Route-test.js | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/react-router/modules/__tests__/Route-test.js b/packages/react-router/modules/__tests__/Route-test.js index 5e810239b4..ad5a306c36 100644 --- a/packages/react-router/modules/__tests__/Route-test.js +++ b/packages/react-router/modules/__tests__/Route-test.js @@ -57,13 +57,20 @@ describe('A ', () => { }) it('supports preact by nulling out children prop when empty array is passed', () => { + const TEXT = 'Mrs. Kato' const node = document.createElement('div') - expect(() => { - ReactDOM.render(( - null} /> - ), node) - }).toThrow(/You should not use or withRoute\(\) outside a valid /) + ReactDOM.render(( + + ( +

{TEXT}

+ )}> + {[]} +
+
+ ), node) + + expect(node.innerHTML).toContain(TEXT) }) it('matches using nextContext when updating', () => { @@ -83,7 +90,13 @@ describe('A ', () => { }) it('crash explicitly with no valid ', () => { + const node = document.createElement('div') + expect(() => { + ReactDOM.render(( + null} /> + ), node) + }).toThrow(/You should not use or withRoute\(\) outside a valid /) }) }) From 14a6bd8b8afbd8fc08087abe3355fcf024eb9329 Mon Sep 17 00:00:00 2001 From: Benoit Tremblay Date: Sun, 9 Apr 2017 15:26:55 -0400 Subject: [PATCH 4/6] Add router check before first usage on --- packages/react-router/modules/Route.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/react-router/modules/Route.js b/packages/react-router/modules/Route.js index 2315540a35..adb855aff0 100644 --- a/packages/react-router/modules/Route.js +++ b/packages/react-router/modules/Route.js @@ -34,10 +34,6 @@ class Route extends React.Component { } getChildContext() { - if (!this.context.router) { - throw new Error('You should not use or withRoute() outside a valid ') - } - return { router: { ...this.context.router, @@ -53,10 +49,15 @@ class Route extends React.Component { match: this.computeMatch(this.props, this.context.router) } - computeMatch({ computedMatch, location, path, strict, exact }, { route }) { + computeMatch({ computedMatch, location, path, strict, exact }, router) { if (computedMatch) return computedMatch // already computed the match for us + if (!router) { + throw new Error('You should not use or withRoute() outside a valid ') + } + + const { route } = router const pathname = (location || route.location).pathname return path ? matchPath(pathname, { path, strict, exact }) : route.match From f6b0325ab7ae4ceb7557fb48111acdcfe9313c61 Mon Sep 17 00:00:00 2001 From: Benoit Tremblay Date: Mon, 10 Apr 2017 22:32:39 -0400 Subject: [PATCH 5/6] Switch throw to invariant for check --- packages/react-router/modules/Route.js | 8 +++++--- packages/react-router/modules/__tests__/Route-test.js | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/react-router/modules/Route.js b/packages/react-router/modules/Route.js index adb855aff0..b41d65964a 100644 --- a/packages/react-router/modules/Route.js +++ b/packages/react-router/modules/Route.js @@ -1,4 +1,5 @@ import warning from 'warning' +import invariant from 'invariant'; import React from 'react' import PropTypes from 'prop-types' import matchPath from './matchPath' @@ -53,9 +54,10 @@ class Route extends React.Component { if (computedMatch) return computedMatch // already computed the match for us - if (!router) { - throw new Error('You should not use or withRoute() outside a valid ') - } + invariant( + router, + 'You should not use or withRouter() outside a valid ' + ) const { route } = router const pathname = (location || route.location).pathname diff --git a/packages/react-router/modules/__tests__/Route-test.js b/packages/react-router/modules/__tests__/Route-test.js index ad5a306c36..a291187b92 100644 --- a/packages/react-router/modules/__tests__/Route-test.js +++ b/packages/react-router/modules/__tests__/Route-test.js @@ -96,7 +96,7 @@ describe('A ', () => { ReactDOM.render(( null} /> ), node) - }).toThrow(/You should not use or withRoute\(\) outside a valid /) + }).toThrow(/You should not use or withRouter\(\) outside a valid /) }) }) From c126da1855205d469cd97671e38d549e9a2faaca Mon Sep 17 00:00:00 2001 From: Benoit Tremblay Date: Mon, 10 Apr 2017 22:50:43 -0400 Subject: [PATCH 6/6] Add check on , , , --- packages/react-router-dom/modules/Link.js | 6 ++++++ .../modules/__tests__/Link-test.js | 10 ++++++++++ packages/react-router-dom/package.json | 1 + packages/react-router/modules/Prompt.js | 6 ++++++ packages/react-router/modules/Redirect.js | 6 ++++++ packages/react-router/modules/Switch.js | 8 ++++++++ .../modules/__tests__/Switch-test.js | 17 +++++++++++++++++ 7 files changed, 54 insertions(+) diff --git a/packages/react-router-dom/modules/Link.js b/packages/react-router-dom/modules/Link.js index c614dd3e65..1dd9f29068 100644 --- a/packages/react-router-dom/modules/Link.js +++ b/packages/react-router-dom/modules/Link.js @@ -1,5 +1,6 @@ import React from 'react' import PropTypes from 'prop-types' +import invariant from 'invariant' const isModifiedEvent = (event) => !!(event.metaKey || event.altKey || event.ctrlKey || event.shiftKey) @@ -58,6 +59,11 @@ class Link extends React.Component { render() { const { replace, to, ...props } = this.props // eslint-disable-line no-unused-vars + invariant( + this.context.router, + 'You should not use outside a valid ' + ) + const href = this.context.router.history.createHref( typeof to === 'string' ? { pathname: to } : to ) diff --git a/packages/react-router-dom/modules/__tests__/Link-test.js b/packages/react-router-dom/modules/__tests__/Link-test.js index 78ce53f5d1..71362964e5 100644 --- a/packages/react-router-dom/modules/__tests__/Link-test.js +++ b/packages/react-router-dom/modules/__tests__/Link-test.js @@ -24,6 +24,16 @@ describe('A ', () => { expect(href).toEqual('/the/path?the=query#the-hash') }) + + it('crash explicitly with no valid ', () => { + const node = document.createElement('div') + + expect(() => { + ReactDOM.render(( + link + ), node) + }).toThrow(/You should not use outside a valid /) + }) }) describe('When a is clicked', () => { diff --git a/packages/react-router-dom/package.json b/packages/react-router-dom/package.json index 4be049dec6..4fa6a746b5 100644 --- a/packages/react-router-dom/package.json +++ b/packages/react-router-dom/package.json @@ -41,6 +41,7 @@ }, "dependencies": { "history": "^4.5.1", + "invariant": "^2.2.2", "loose-envify": "^1.3.1", "prop-types": "^15.5.4", "react-router": "^4.0.0" diff --git a/packages/react-router/modules/Prompt.js b/packages/react-router/modules/Prompt.js index 3c875798b6..7fbfaec23f 100644 --- a/packages/react-router/modules/Prompt.js +++ b/packages/react-router/modules/Prompt.js @@ -1,5 +1,6 @@ import React from 'react' import PropTypes from 'prop-types' +import invariant from 'invariant' /** * The public API for prompting the user before navigating away @@ -41,6 +42,11 @@ class Prompt extends React.Component { } componentWillMount() { + invariant( + this.context.router, + 'You should not use outside a valid ' + ) + if (this.props.when) this.enable(this.props.message) } diff --git a/packages/react-router/modules/Redirect.js b/packages/react-router/modules/Redirect.js index 416c923e6c..fb31251953 100644 --- a/packages/react-router/modules/Redirect.js +++ b/packages/react-router/modules/Redirect.js @@ -1,5 +1,6 @@ import React from 'react' import PropTypes from 'prop-types' +import invariant from 'invariant' /** * The public API for updating the location programatically @@ -34,6 +35,11 @@ class Redirect extends React.Component { } componentWillMount() { + invariant( + this.context.router, + 'You should not use outside a valid ' + ) + if (this.isStatic()) this.perform() } diff --git a/packages/react-router/modules/Switch.js b/packages/react-router/modules/Switch.js index 1fd43530e3..1d82f1f960 100644 --- a/packages/react-router/modules/Switch.js +++ b/packages/react-router/modules/Switch.js @@ -1,6 +1,7 @@ import React from 'react' import PropTypes from 'prop-types' import warning from 'warning' +import invariant from 'invariant' import matchPath from './matchPath' /** @@ -18,6 +19,13 @@ class Switch extends React.Component { location: PropTypes.object } + componentWillMount() { + invariant( + this.context.router, + 'You should not use outside a valid ' + ) + } + componentWillReceiveProps(nextProps) { warning( !(nextProps.location && !this.props.location), diff --git a/packages/react-router/modules/__tests__/Switch-test.js b/packages/react-router/modules/__tests__/Switch-test.js index 1cf1bf71e0..c87f8b51b3 100644 --- a/packages/react-router/modules/__tests__/Switch-test.js +++ b/packages/react-router/modules/__tests__/Switch-test.js @@ -135,6 +135,23 @@ describe('A ', () => { expect(node.innerHTML).toMatch(/one/) }) + + it('crash explicitly with no valid ', () => { + const node = document.createElement('div') + + expect(() => { + ReactDOM.render(( + + ( +

one

+ )}/> + ( +

two

+ )}/> +
+ ), node) + }).toThrow(/You should not use outside a valid /) + }) })