Skip to content
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

Make <Link> work with static containers #3429

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
94 changes: 81 additions & 13 deletions modules/Link.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@ function isEmptyObject(object) {
return true
}

function isLinkActive(router, props) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These conditions are moved from the render function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this make the link always re-render twice on location updates then? Once with the old active state, once with the recalculated active state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing this doesn’t seem to be the case. I think the listener fires before React has a chance to process setState in the router so it looks like everything happens in a single batch. I might be wrong about the exact mechanics though—just saying I don’t see duplicate renders in my example app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note my caveat in #470 (comment) – if you hit this from clicking on a link, then stuff might well get batched together, but it won't happen if the transition is triggered from something other than a React event handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. I get it now.

// Ignore if rendered outside the context of router, simplifies unit testing.
if (!router) {
return false
}

// Ignore if the render output is unaffected by the active state.
const { to, activeClassName, activeStyle, onlyActiveOnIndex } = props
if (!activeClassName && (activeStyle == null || isEmptyObject(activeStyle))) {
return false
}

return router.isActive(to, onlyActiveOnIndex)
}

/**
* A <Link> is used to create an <a> element that links to a route.
* When that route is active, the link gets the value of its
Expand Down Expand Up @@ -62,6 +77,60 @@ const Link = React.createClass({
}
},

getInitialState() {
return {
isActive: isLinkActive(this.context.router, this.props)
}
},

componentWillMount() {
this._isActive = this.state.isActive
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’re going to use this._isActive for the pending state from now on. This lets us quickly bail out if it hasn’t changed. (See updateIsActive later.) This is a performance optimization.

},

componentDidMount() {
const { router } = this.context
if (router) {
this._unlisten = router.listen(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR switches router.listen to be implemented by the transition manager so it reports the tree about to be rendered.

if (this._unlisten) {
this.updateIsActive()
}
})
}
},

componentWillReceiveProps(nextProps) {
const { router } = this.context
if (router) {
if (
nextProps.to !== this.props.to ||
nextProps.onlyActiveOnIndex !== this.props.onlyActiveOnIndex ||
nextProps.activeClassName !== this.props.activeClassName ||
nextProps.activeStyle !== this.props.activeStyle
) {
this.updateIsActive(nextProps)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to updateIsActive if these props changed. Performance optimization.

}
}
},

componentWillUnmount() {
if (this._unlisten) {
this._unlisten()
this._unlisten = null
}
},

updateIsActive(props = this.props) {
const { router } = this.context
const isActive = isLinkActive(router, props)

// The code is written this way to avoid wasted
// setState() calls that get expensive in large trees.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The perf problem isn't setState() – the slow call is router.isActive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but we only call it once initially (which we would call anyway), and when the related props have changed (which again, we’d have to recompute anyway).

setState() however can be slow on big trees. I’ve seen 3x difference on large trees in other project solved by avoiding setState even despite further shouldComponentUpdate bailout. React places setState in a queue and marks subtrees as dirty so this might be relevant.

Copy link
Contributor Author

@gaearon gaearon May 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Previously, it would be called during render, but now it doesn’t, so it seems like the number of useful times is equivalent. As far as I can see any other wins from not calling it are the bug, not the feature 😄 )

if (isActive !== this._isActive) {
this._isActive = isActive
this.setState({ isActive: this._isActive })
}
},

handleClick(event) {
let allowTransition = true

Expand Down Expand Up @@ -93,26 +162,25 @@ const Link = React.createClass({
},

render() {
const { to, activeClassName, activeStyle, onlyActiveOnIndex, ...props } = this.props
// Ignore if rendered outside the context of router, simplifies unit testing.
const { to, activeClassName, activeStyle, ...props } = this.props
const { router } = this.context
const { isActive } = this.state

// Ignore if rendered outside the context of router, simplifies unit testing.
if (router) {
props.href = router.createHref(to)

if (activeClassName || (activeStyle != null && !isEmptyObject(activeStyle))) {
if (router.isActive(to, onlyActiveOnIndex)) {
if (activeClassName) {
if (props.className) {
props.className += ` ${activeClassName}`
} else {
props.className = activeClassName
}
if (isActive) {
if (activeClassName) {
if (props.className) {
props.className += ` ${activeClassName}`
} else {
props.className = activeClassName
}

if (activeStyle)
props.style = { ...props.style, ...activeStyle }
}

if (activeStyle)
props.style = { ...props.style, ...activeStyle }
}
}

Expand Down
1 change: 1 addition & 0 deletions modules/RouterUtils.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export function createRouterObject(history, transitionManager) {
return {
...history,
listen: transitionManager.listen,
setRouteLeaveHook: transitionManager.listenBeforeLeavingRoute,
isActive: transitionManager.isActive
}
Expand Down
41 changes: 41 additions & 0 deletions modules/__tests__/Link-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,47 @@ describe('A <Link>', function () {
</Router>
), node, execNextStep)
})

it('changes active state inside static components', function (done) {
class LinkWrapper extends Component {
shouldComponentUpdate() {
return false
}

render() {
return (
<div>
<Link to="/hello" activeClassName="active">Link</Link>
{this.props.children}
</div>
)
}
}

let a
const history = createHistory('/goodbye')
const steps = [
function () {
a = node.querySelector('a')
expect(a.className).toEqual('')
history.push('/hello')
},
function () {
expect(a.className).toEqual('active')
}
]

const execNextStep = execSteps(steps, done)

render((
<Router history={history} onUpdate={execNextStep}>
<Route path="/" component={LinkWrapper}>
<Route path="goodbye" component={Goodbye} />
<Route path="hello" component={Hello} />
</Route>
</Router>
), node, execNextStep)
})
})

describe('when clicked', function () {
Expand Down
82 changes: 60 additions & 22 deletions modules/createTransitionManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,35 +209,73 @@ export default function createTransitionManager(history, routes) {
}
}

let ownListeners = []
let unlistenHistory
let lastHistoryLocation

function handleHistoryLocationChange(location, callback) {
if (state.location === location) {
callback(null, state)
} else {
match(location, function (error, redirectLocation, nextState) {
if (error) {
callback(error)
} else if (redirectLocation) {
history.transitionTo(redirectLocation)
} else if (nextState) {
callback(null, nextState)
} else {
warning(
false,
'Location "%s" did not match any routes',
location.pathname + location.search + location.hash
)
}
})
}
}

function notifyOwnListeners(error, nextState) {
ownListeners.forEach(listener => listener(error, nextState))
}

/**
* This is the API for stateful environments. As the location
* changes, we update state and call the listener. We can also
* gracefully handle errors and redirects.
*/
function listen(listener) {
// TODO: Only use a single history listener. Otherwise we'll
// end up with multiple concurrent calls to match.
return history.listen(function (location) {
if (state.location === location) {
listener(null, state)
} else {
match(location, function (error, redirectLocation, nextState) {
if (error) {
listener(error)
} else if (redirectLocation) {
history.transitionTo(redirectLocation)
} else if (nextState) {
listener(null, nextState)
} else {
warning(
false,
'Location "%s" did not match any routes',
location.pathname + location.search + location.hash
)
}
})
if (!unlistenHistory) {
// Set up a shared subscription to history
// when the first own listener subscribes.
unlistenHistory = history.listen(location => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation uses a shared listener that gets subscribed and unsubscribed lazily on first/last listener.

lastHistoryLocation = location
handleHistoryLocationChange(location, notifyOwnListeners)
})
}

ownListeners.push(listener)

// Since history.listen() only happens once,
// we manually call the new listener synchronously.
handleHistoryLocationChange(lastHistoryLocation, listener)

return () => {
const index = ownListeners.indexOf(listener)
if (index === -1) {
// This listener has been unsubscribed before
return
}
})

ownListeners.splice(index, 1)

// Tear down the shared subscription to history
// when the last own listener unsubscribes.
if (!ownListeners.length) {
unlistenHistory()
unlistenHistory = null
}
}
}

return {
Expand Down