Skip to content

Proposal: non-blocking updates refactor #5534

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

Closed
wants to merge 1 commit into from
Closed
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
45 changes: 24 additions & 21 deletions packages/react-router/modules/Route.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const isEmptyChildren = (children) =>
/**
* The public API for matching a single path and rendering.
*/
class Route extends React.Component {
class Route extends React.PureComponent {
static propTypes = {
computedMatch: PropTypes.object, // private, from <Switch>
path: PropTypes.string,
Expand All @@ -29,25 +29,12 @@ class Route extends React.Component {
static contextTypes = {
router: PropTypes.shape({
history: PropTypes.object.isRequired,
route: PropTypes.object.isRequired,
staticContext: PropTypes.object
})
}

static childContextTypes = {
router: PropTypes.object.isRequired
}

getChildContext() {
return {
router: {
...this.context.router,
route: {
location: this.props.location || this.context.router.route.location,
match: this.state.match
}
}
}
static defaultProps = {
path: '/'
}

state = {
Expand All @@ -63,13 +50,15 @@ class Route extends React.Component {
'You should not use <Route> or withRouter() outside a <Router>'
)

const { route } = router
const pathname = (location || route.location).pathname
const { history } = router
const pathname = (location || history.location).pathname

return path ? matchPath(pathname, { path, strict, exact, sensitive }) : route.match
return path ? matchPath(pathname, { path, strict, exact, sensitive }) : null
}

componentWillMount() {
const { history } = this.context.router

warning(
!(this.props.component && this.props.render),
'You should not use <Route component> and <Route render> in the same route; <Route render> will be ignored'
Expand All @@ -84,6 +73,12 @@ class Route extends React.Component {
!(this.props.render && this.props.children && !isEmptyChildren(this.props.children)),
'You should not use <Route render> and <Route children> in the same route; <Route children> will be ignored'
)

this.unlisten = history.listen(() => {
this.setState({
match: this.computeMatch(this.props, this.context.router)
})
})
}

componentWillReceiveProps(nextProps, nextContext) {
Expand All @@ -102,11 +97,19 @@ class Route extends React.Component {
})
}

shouldComponentUpdate(nextProps, nextState) {
return !this.state.match || !nextState.match || this.state.match.url !== nextState.match.url
}

componentWillUnmount() {
this.unlisten()
}

render() {
const { match } = this.state
const { children, component, render } = this.props
const { history, route, staticContext } = this.context.router
const location = this.props.location || route.location
const { history, staticContext } = this.context.router
const location = this.props.location || history.location
const props = { match, location, history, staticContext }

if (component)
Expand Down
34 changes: 2 additions & 32 deletions packages/react-router/modules/Router.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,44 +24,18 @@ class Router extends React.Component {
return {
router: {
...this.context.router,
history: this.props.history,
route: {
location: this.props.history.location,
match: this.state.match
}
history: this.props.history
}
}
}

state = {
match: this.computeMatch(this.props.history.location.pathname)
}

computeMatch(pathname) {
return {
path: '/',
url: '/',
params: {},
isExact: pathname === '/'
}
}

componentWillMount() {
const { children, history } = this.props
const { children } = this.props

invariant(
children == null || React.Children.count(children) === 1,
'A <Router> may have only one child element'
)

// Do this here so we can setState when a <Redirect> changes the
// location in componentWillMount. This happens e.g. when doing
// server rendering using a <StaticRouter>.
this.unlisten = history.listen(() => {
this.setState({
match: this.computeMatch(history.location.pathname)
})
})
}

componentWillReceiveProps(nextProps) {
Expand All @@ -71,10 +45,6 @@ class Router extends React.Component {
)
}

componentWillUnmount() {
this.unlisten()
}

render() {
const { children } = this.props
return children ? React.Children.only(children) : null
Expand Down
30 changes: 12 additions & 18 deletions packages/react-router/modules/Switch.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,38 +10,32 @@ import matchPath from './matchPath'
class Switch extends React.Component {
static contextTypes = {
router: PropTypes.shape({
route: PropTypes.object.isRequired
history: PropTypes.object.isRequired
}).isRequired
}

static propTypes = {
children: PropTypes.node,
location: PropTypes.object
children: PropTypes.node
}

componentWillMount() {
invariant(
this.context.router,
'You should not use <Switch> outside a <Router>'
)
}

componentWillReceiveProps(nextProps) {
warning(
!(nextProps.location && !this.props.location),
'<Switch> elements should not change from uncontrolled to controlled (or vice versa). You initially used no "location" prop and then provided one on a subsequent render.'
)
this.unlisten = this.context.router.history.listen(() => {
this.forceUpdate();
})
}

warning(
!(!nextProps.location && this.props.location),
'<Switch> elements should not change from controlled to uncontrolled (or vice versa). You provided a "location" prop initially but omitted it on a subsequent render.'
)
componentWillUnmount() {
this.unlisten();
}

render() {
const { route } = this.context.router
const { history: { location } } = this.context.router
const { children } = this.props
const location = this.props.location || route.location

let match, child
React.Children.forEach(children, element => {
Expand All @@ -50,13 +44,13 @@ class Switch extends React.Component {
const { path: pathProp, exact, strict, sensitive, from } = element.props
const path = pathProp || from

if (match == null) {
if (!match) {
child = element
match = path ? matchPath(location.pathname, { path, exact, strict, sensitive }) : route.match
match = !path || matchPath(location.pathname, { path, exact, strict, sensitive })
}
})

return match ? React.cloneElement(child, { location, computedMatch: match }) : null
return match ? child : null
}
}

Expand Down
140 changes: 52 additions & 88 deletions packages/react-router/modules/__tests__/Route-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,6 @@ describe('A <Route>', () => {
expect(node.innerHTML).not.toContain(TEXT)
})

it('can use a `location` prop instead of `context.router.route.location`', () => {
const TEXT = 'tamarind chutney'
const node = document.createElement('div')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/mint' ]}>
<Route
location={{ pathname: '/tamarind' }}
path="/tamarind"
render={() => (
<h1>{TEXT}</h1>
)}
/>
</MemoryRouter>
), node)

expect(node.innerHTML).toContain(TEXT)
})

it('supports preact by nulling out children prop when empty array is passed', () => {
const TEXT = 'Mrs. Kato'
const node = document.createElement('div')
Expand Down Expand Up @@ -97,6 +78,58 @@ describe('A <Route>', () => {
), node)
}).toThrow(/You should not use <Route> or withRouter\(\) outside a <Router>/)
})

it('updates itself even when the parent blocks an update', () => {
const node = document.createElement('div')

class Blocking extends React.Component {
shouldComponentUpdate() {
return false
}

render() {
return this.props.children
}
}

let push
ReactDOM.render((
<MemoryRouter initialEntries={[ '/sushi/california' ]}>
<Blocking>
<Route path="/sushi/:roll" render={({ history, match }) => {
push = history.push
return <div>{match.url}</div>
}}/>
</Blocking>
</MemoryRouter>
), node)
push('/sushi/dynamite')
expect(node.innerHTML).toContain('/sushi/dynamite')
})

it('updates itself even when the match is different', () => {
const node = document.createElement('div')

let push
let count = 0
ReactDOM.render((
<MemoryRouter initialEntries={[ '/sushi/california' ]}>
<Route path="/sushi/:roll" render={({ history, match }) => {
push = history.push
count = count + 1
return <div>{count}</div>
}}/>
</MemoryRouter>
), node)
push('/sushi/dynamite')
expect(node.innerHTML).toContain(2)
push('/sushi/dynamite')
expect(node.innerHTML).toContain(2)
push('/sushi/dynamite/eat')
expect(node.innerHTML).toContain(2)
push('/sushi/california')
expect(node.innerHTML).toContain(3)
})
})

describe('A <Route> with dynamic segments in the path', () => {
Expand Down Expand Up @@ -309,72 +342,3 @@ describe('A <Route exact strict>', () => {
expect(node.innerHTML).not.toContain(TEXT)
})
})

describe('A <Route location>', () => {
it('can use a `location` prop instead of `router.location`', () => {
const TEXT = 'tamarind chutney'
const node = document.createElement('div')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/mint' ]}>
<Route
location={{ pathname: '/tamarind' }}
path="/tamarind"
render={() => (
<h1>{TEXT}</h1>
)}
/>
</MemoryRouter>
), node)

expect(node.innerHTML).toContain(TEXT)
})

describe('children', () => {
it('uses parent\'s prop location', () => {
const TEXT = 'cheddar pretzel'
const node = document.createElement('div')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/popcorn' ]}>
<Route
location={{ pathname: '/pretzels/cheddar' }}
path="/pretzels"
render={() => (
<Route path='/pretzels/cheddar' render={() => (
<h1>{TEXT}</h1>
)} />
)}
/>
</MemoryRouter>
), node)

expect(node.innerHTML).toContain(TEXT)
})

it('continues to use parent\'s prop location after navigation', () => {
const TEXT = 'cheddar pretzel'
const node = document.createElement('div')
let push
ReactDOM.render((
<MemoryRouter initialEntries={[ '/popcorn' ]}>
<Route
location={{ pathname: '/pretzels/cheddar' }}
path="/pretzels"
render={({ history }) => {
push = history.push
return (
<Route path='/pretzels/cheddar' render={() => (
<h1>{TEXT}</h1>
)} />
)
}}
/>
</MemoryRouter>
), node)
expect(node.innerHTML).toContain(TEXT)
push('/chips')
expect(node.innerHTML).toContain(TEXT)
})
})
})
Loading