props.activeRoute not re-rendered #34

hendrikcech · 2014-06-19T22:08:12Z

Hey, I've encountered a bug while using this library and attached a failing test case.

It seems like the props.activeRoute component is not re-rendered when calling router.renderComponent().

The first render call behaves correctly. After that I expect 'render index' and 'render app' to be logged every second. Instead only render index appears. The timestamp doesn't change either.

I'm not really sure where to start looking for the bug. Maybe you could give me a hint?

browserify -t reactify main.jsx > bundle.js

var React = require('react')
var RRouter = require('react-nested-router')
var Router = RRouter.Router
var Route = RRouter.Route

var App = React.createClass({
    render: function() {
        console.log('render app')
        return (
            <div>
                <h1>App</h1>
                {this.props.activeRoute}
            </div>
        )
    }
})

var Index = React.createClass({
    render: function() {
        console.log('render index')
        return <span>{Date.now()}</span>
    }
})

var router = Router(
    <Route handler={App}>
        <Route name='index' path='/' handler={Index} />
    </Route>
)

setInterval(function() {
    router.renderComponent(document.body)
}, 1000)

<!DOCTYPE html>
<body></body>
<script src='bundle.js'></script>

The text was updated successfully, but these errors were encountered:

mjackson · 2014-06-20T00:11:54Z

Rendering the same component to the same element in React is essentially a no-op if nothing else has changed. To trigger another render, you should try changing some state and/or props in your App component.

hendrikcech · 2014-06-20T07:26:11Z

React should still call the render function of the Index component first. Only after that it should compare the output and decide if the real DOM needs to be updated.

Consider the following setup. It is basically the same example with a static Index route and behaves as I expected.

I'm managing data outside of React and just pass getter functions as props to components. I therefor need a way to force a re-render when data changes.
I was using an earlier version of this library where this used to work. The version released to npm yesterday broke this.

var React = require('react')
var RRouter = require('react-nested-router')
var Router = RRouter.Router
var Route = RRouter.Route

var App = React.createClass({
    render: function() {
        console.log('render app')
        return (
            <div>
                <h1>App</h1>
                <Index />
            </div>
        )
    }
})

var Index = React.createClass({
    render: function() {
        console.log('render index')
        return <span>{Date.now()}</span>
    }
})

var router = Router(<Route handler={App} />)

setInterval(function() {
    router.renderComponent(document.body)
}, 1000)

mjackson · 2014-06-20T14:42:06Z

Why would it call the render method of Index if no props/state have changed?

hendrikcech · 2014-06-20T15:02:31Z

Because React can't know if props changed. That's why it calls the render function of all nodes, even when they neither receive props nor have state. Look at this example.

ryanflorence · 2014-06-20T15:02:32Z

looks like react does in fact rerender even if the state hasn't changed:

http://jsbin.com/henuz/1/edit

mjackson · 2014-06-20T15:57:14Z

@hendrikcech Thanks for the example. I'm reopening this issue, tho I'm not sure how to address it. Our implementation of Router#renderComponent simply does some setup and delegates directly to React.renderComponent under the hood, so I'm not sure what's preventing the re-render.

hendrikcech · 2014-06-20T18:28:33Z

I'm not sure yet either. I looked into the commit history and now know that it used to work correctly up until d83cf7b. After that different bugs come up. Some builds throw this error: "Uncaught Error: Invariant Violation: Rendering the same router multiple times is not supported". fbedbca introduces the current bug by removing the check.
This propably doesn't help, but well, there you have it.

ryanflorence · 2014-06-20T20:10:27Z

I can imagine this will mess up integration tests where you want to render an app, tear it down, and then do it again.

mjackson · 2014-06-20T22:12:25Z

@hendrikcech I removed the `invariant

mjackson · 2014-06-20T22:13:30Z

@hendrikcech Oops. Sent too soon.

I removed the invariant because after reading the React docs it seemed like rendering the same component twice to the same element was supported, so I opted to support it in Router#renderComponent as well.

hendrikcech · 2014-06-20T22:54:47Z

Sorry, I phrased that badly. What I meant was that the bug first showed there because the check was removed. Rendering a component multiple times to the same element should be supported.

I couldn't nail down the issue yet but have a theory. Maybe not the constructor of the active route is passed in props.activeRoute but the mounted instance. Like in this example.

mjackson · 2014-06-24T05:25:03Z

@hendrikcech We have deprecated/removed our own custom version of renderComponent and now recommend that you use React.renderComponent directly with a <Route> component (see the updated docs). I'm assuming this issue had something to do with our rendering, so I'm hoping this resolves the issue since rendering a <Route> is the same as rendering any other component.

Can you confirm/deny?

hendrikcech · 2014-06-24T11:39:42Z

Unfortunately this doesn't solve the problem. I have no clue where the bug originates from.
For now this limits the usefulness of this router since it can't be used with libraries like cortex.

mjackson · 2014-06-24T15:09:09Z

@hendrikcech What does the code look like that you're using to test the issue with version 0.2.0?

hendrikcech · 2014-06-24T15:39:46Z

@mjackson I'm this using this code:

var React = require('react')
var Route = require('react-nested-router').Route

var App = React.createClass({
    render: function() {
        console.log('render app')
        return (
            <div>
                <h1>App: {Date.now()}</h1>
                {this.props.activeRoute}
            </div>
        )
    }
})

var Index = React.createClass({
    render: function() {
        console.log('render index')
        return <span>Index: {Date.now()}</span>
    }
})

var router = (
    <Route handler={App}>
        <Route name='index' path='/' handler={Index} />
    </Route>
)

setInterval(function() {
    React.renderComponent(router, document.body)
}, 1000)

The router doesn't work well with jsfiddle, otherwise I would have put it up there.

sophiebits · 2014-06-25T05:17:09Z

I haven't read the source enough to know if this is the case, but if you're passing the same descriptor then React will skip reconciliation altogether. If you recreate the descriptor (with a new props object, even if the props are all the same) it'll redo the reconciliation.

hendrikcech · 2014-06-25T07:32:05Z

@spicyj That was my first guess as well. However, as far as I can tell, the descriptor is recreated each time.

mjackson · 2014-06-25T14:24:28Z

@hendrikcech That code runs each time a route's props are recalculated. However, in your example code the route never changes, so props remain the same. You're re-using the same descriptor for your top-level Route in each call to React.renderComponent.

hendrikcech · 2014-06-25T15:06:23Z

@mjackson I didn't realize that. Recalculating a route's props on each render call fixes the issue. I got the example working by changing the render function in Route.js to the following code.

render: function () {
  var path = URLStore.getCurrentPath();
  var nextMatches = this.match(path);
  var query = Path.extractQuery(path) || {};
  var handlerProps = computeHandlerProps(nextMatches, query);
  return this.props.handler(handlerProps);
}

You may be able to find a smarter way to do this. I'm not sure if this raises new problems.

mjackson · 2014-06-25T15:23:09Z

I think the core issue here may be that you're used to creating descriptors inside a render function, but here we're recommending you re-use the same descriptor.

Another way to get your example to work (untested) may be to do something like this:

function getRoutes() {
  return <Route ... />;
}

setInterval(function() {
    React.renderComponent(getRoutes(), document.body)
}, 1000)

This way you'll be passing a new descriptor to React.renderComponent every second.

hendrikcech · 2014-06-25T15:36:43Z

@mjackson That doesn't work. The state of the root Route component, which caches the props.activeRoute descriptor, isn't updated unless the path changes. Calling this.dispatch() directly doesn't work either since syncWithTransition aborts if the path didn't change.

mjackson · 2014-06-25T23:03:58Z

@hendrikcech Does this commit fix this issue? Instead of hanging onto a handler's props in between invocations of render, we're now calculating them on-the-fly.

hendrikcech · 2014-06-26T15:25:48Z

@mjackson Nice, the example works with the latest commit. Thanks!

mjackson · 2014-06-27T06:21:26Z

@hendrikcech Glad we got it fixed. Thanks for following up!

mjackson closed this as completed Jun 20, 2014

mjackson reopened this Jun 20, 2014

mjackson mentioned this issue Jun 21, 2014

make Router a React component #38

Closed

ryanflorence added the bug label Jun 21, 2014

mjackson mentioned this issue Jun 21, 2014

Route component #40

Merged

hendrikcech closed this as completed Jun 26, 2014

lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

props.activeRoute not re-rendered #34

props.activeRoute not re-rendered #34

hendrikcech commented Jun 19, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

ryanflorence commented Jun 20, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

ryanflorence commented Jun 20, 2014

mjackson commented Jun 20, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

mjackson commented Jun 24, 2014

hendrikcech commented Jun 24, 2014

mjackson commented Jun 24, 2014

hendrikcech commented Jun 24, 2014

sophiebits commented Jun 25, 2014

hendrikcech commented Jun 25, 2014

mjackson commented Jun 25, 2014

hendrikcech commented Jun 25, 2014

mjackson commented Jun 25, 2014

hendrikcech commented Jun 25, 2014

mjackson commented Jun 25, 2014

hendrikcech commented Jun 26, 2014

mjackson commented Jun 27, 2014

props.activeRoute not re-rendered #34

props.activeRoute not re-rendered #34

Comments

hendrikcech commented Jun 19, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

ryanflorence commented Jun 20, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

ryanflorence commented Jun 20, 2014

mjackson commented Jun 20, 2014

mjackson commented Jun 20, 2014

hendrikcech commented Jun 20, 2014

mjackson commented Jun 24, 2014

hendrikcech commented Jun 24, 2014

mjackson commented Jun 24, 2014

hendrikcech commented Jun 24, 2014

sophiebits commented Jun 25, 2014

hendrikcech commented Jun 25, 2014

mjackson commented Jun 25, 2014

hendrikcech commented Jun 25, 2014

mjackson commented Jun 25, 2014

hendrikcech commented Jun 25, 2014

mjackson commented Jun 25, 2014

hendrikcech commented Jun 26, 2014

mjackson commented Jun 27, 2014