Unnecessary re-rendering under bailed-out components when a legacy context provider and a deep child are updated in the same batch

[iamdustan]

This issue is going to start off mostly theoretical as I’m still working to make a minimal repro case.

We have a scenario where one component is having shouldComponentUpdate() return false to bail out, but a child component is still having its render method called.

Avoiding many details this is roughly what we have:

import React, { Component } from 'react';

class A extends Component {
  shouldComponentUpdate(nextProps) {
    const result = Boolean(nextProps.item);
    console.log('A#shouldComponentUpdate?', result);
    return result;
  }

  render() {
    console.log('A#render', this.props.item);
    return <B item={this.props.item} />;
  }
}

class B extends Component {
  state = { seconds: 0 };
  componentDidMount() {
    this._interval = setInterval(
      () => this.setState({ seconds: this.state.seconds + 1 }),
      1000
    );
  }

  componentWillUnMount() {
    clearInterval(this._interval);
  }

  render() {
    console.log('B#render', this.props.item);
    return (
      <div>
        <strong>{this.props.item.name}</strong>
        <span>{this.state.seconds} seconds</span>
      </div>
    );
  }
}

export default A;

While this case does work as expected it seems to be in the direction of the
errors we’re seeing.

There is something taking place in our render cycle where B is being rendered
without reusing the item prop from the previous reconcile.

My first question is are there any theories on why this may be happening that I
can explore? We are using context as the parent of A and asB and these
are reading from a flux-thing (I think a fork of the original OSS Flux), they
they these are both passing all props through and not having any naming
collisions. I’m fairly certain we are not performing any mutations on our end.

(If I do manage to pull off a repro case I will immediately post it here with
utter joy in my heart)

cc @acdlite @gaearon (this is the issue I was asking about in Messenger recently)

[gaearon]

but then a child component.

You're leaving me in suspense 😀

[wayne-liberty]

I am having the same problem: children get updated while container's shouldComponentUpdate return false. Didn't happened in version15.

[gaearon]

@gespiton Unfortunately this is not helpful without a reproducing example.

[wayne-liberty]

@gaearon here is code repo, it might seem over complicated. the point is, every time I type, all the children re-rendered(the log says it). you have to open main.css then toggle porject view to see the complete page it would be very nice of you to help me with this

[gaearon]

This code looks very odd:

  constructor(props) {
    super(props);
    this.state = props;
  }

Why are you doing this?

[wayne-liberty]

oh.... I've some misunderstanding with props and state, I'll change them later, but is this the problem? maybe I should make a simplified repo?

[gaearon]

Yes, further simplifying would be great!

[iamdustan]

I’m going to close this. We’re 90% certain we’ve tracked down the issue to react-broadcast.

react-broadcast provides a reliable way for React components to propagate state changes to their descendants deep in the component hierarchy, bypassing intermediaries who return false from shouldComponentUpdate.

There still appears to be a subtle difference between React 15 and 16 with React Broadcast’s sCU skipping, but I have been utterly unable to make a minimal repro and it’s only appeared in one very small edge case of our products so 🤷‍♂️

[johanobergman]

@iamdustan Did you find a solution to your problem?

@gaearon I made a simple reproduction of the issue here https://github.com/johanobergman/react-context-render-bug/blob/master/src/index.js.

It seems like updates from a top level container that are blocked by an intermediate shouldComponentUpdate still continues down the hierarchy - if and only if a child somewhere deeper sets its state based on a listener attached to context.

Seems really wierd but I started noticing this when I made my react-router links update using this subscription model.

[gaearon]

Can you turn this into a failing unit test for the React repo?

[johanobergman]

I could try. I've further narrowed it down and it seems like it happens when a child and parent component are updated within the same tick, and the parent defines childContextTypes.

[johanobergman]

I've added a failing test in my repo: https://github.com/johanobergman/react-context-render-bug/blob/master/src/index.test.js, but I'm not sure where I'd put it in the React repo. Would you mind having a look?

[gaearon]

You can search for files ending with -test.js that contain getChildContext :-)

[aweary]

Here's a hosted version of the repro using the latest release (16.4.1). You can see that the issue is still occurring.

[gaearon]

Yeah this seems to be a regression introduced in 16.0.
#13409

[gaearon]

@iamdustan

Did you end up working around this somehow?

[alexreardon]

I stumbled into this one also. It almost broke me!

[gaearon]

I looked into fixing this but it's pretty convoluted and didn't feel like it's worth going down the rabbit hole. We intend to make it easier to upgrade to new context, and after that we should deprecate the legacy one.

[alexreardon]

Makes sense @gaearon. I am just relieved that I found this was an actual issue and that I was not going insane.

[johanobergman]

@alexreardon You can work around this behaviour by wrapping some of your setStates in child components in setTimeout. I had them update in response to an event emitter, so I wrapped the entire event publishing in a single setTimeout.

[zxti]

I just wasted a bunch of time tracking this down as well.

It's worth noting: yarn add react-router would have installed v4.3.1 up until only a month ago, and that version uses the legacy context API, so it hasn't been that hard to encounter this, and it can be reasonably tricky to debug. We ourselves had actually encountered this a number of times before (just never tracked it down).

[nataliemarleny]

For completeness, I've bisected to commit 764531d as the one which has introduced this regression. It looks like a change that cannot be simply reverted.

[gaearon]

Yeah that’s when we switched over to the new implementation of React. The bug has likely been there since the beginning.

As far as I’m aware there is no easy fix for this, and using the legacy context in general is discouraged. It’s already deprecated in the Strict Mode, for example.

Now that we have an easier way to access the official Context API from both classes (via contextType) and functions (via useContext), that’s the recommended fix. I’ll close this issue as it’s unlikely this will ever get fixed for legacy context, and I don’t want to give a false impression.

Feel free to continue the discussion though.