Concern for a possible memory leak and performance issue.

[death667b]

In your new updated version of RouteNavItem.js you have put a lambda in the render method.

The problem with these types methods is that they can not be referenced a second time causing them to be recreated in memory. Depending on how often this is being rendered this can cause a significant memory leak and performance hit.

I recommend assigning the method's memory address to a container. const clickHandler = (e) => { ... }

[jayair]

@death667b Good catch. I'll update it shortly.

[jayair]

@death667b Now that I think about it, in this case re-writting it with an explicit reference doesn't necessarily help. But maybe you have something else in mind.

Do you want to take a swipe at re-writting it?

export default props =>
  <Route
    path={props.href}
    exact
    children={({ match, history }) =>
        <NavItem
          onClick={e => history.push(e.currentTarget.getAttribute("href"))}
          {...props}
          active={match ? true : false}
        >
          {props.children}
        </NavItem>}
  />;

[death667b]

I have created a pull request at #9 This is to master.

Any comments on the request is welcome.

[jayair]

Thanks for submitting the PR. Just posting the change here for reference.

const handleClick = (history, event) => {
    history.push(event.currentTarget.getAttribute("href"))
}

export default props =>
  <Route
    path={props.href}
    exact
    children={({ match, history }) =>
      <NavItem
        onClick={handleClick.bind(this, history)}
        {...props}
        active={match ? true : false}
      >
        {props.children}
      </NavItem>}
  />;

In this case the handleClick.bind does not help us because it creates a new function on each call just like creating an anonymous function. There are some small performance differences between .bind and creating an anonymous function but they vary between browser engines and is too small to worry about (https://jsperf.com/simple-null-bind-vs-closure).

The case where the .bind is helpful is where it is called once. Usually you see this in the constructor of a component. It would look something like this.handleClick = this.handleClick.bind(this);, where we would use this newly bound handleClick in place of the old one. But we are only calling .bind once per component creation.

The way I would optimize this method would be to wrap NavItem inside a component that checks if match or history has changed in shouldComponentUpdate or alternatively, use a PureComponent. But as suggested by the React docs the default behavior of rendering on each state change is fine for most cases. So this optimization is more of an advanced topic that I think we could cover separately.

[death667b]

Thanks @jayair for the detailed reply, it is helpful.