props.activeRoute not re-rendered

[remix-run]

props.activeRoute not re-rendered

[mjackson]

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]

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]

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

[hendrikcech]

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]

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

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

[mjackson]

@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]

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]

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]

@hendrikcech I removed the `invariant

[mjackson]

@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]

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]

@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]

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]

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

[hendrikcech]

@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]

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]

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

[mjackson]

@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]

@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]

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]

@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]

@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]

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

[mjackson]

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