Warn in dev if shouldComponentUpdate is defined on PureComponent

[misoguy]

Fix #9239

[aweary]

Hey @misoguy! Thanks for taking this. I'm going to reply to your comment here so we can track the work.

After writing a simple test code, I realized that there is a test code to ensure shouldComponentUpdate can be overridden.
What is the expected behavior when it is overridden?

We specifically want to warn when a user defines shouldComponentUpdate when using React.PureComponent, since PureComponent is meant to implement its own version of shouldComponentUpdate

We can place this warning in ReactCompositeComponent for the stack reconciler. You can use the isPureComponent utility to check if a component is using PureComponent and place the warning with the other dev-only warnings.

For Fiber maybe you can add the warning in ReactFiberClassComponent in checkShouldComponentUpdate . It doesn't look like there's an isPureComponent utility available here, but you can check type.prototype.isPureReactComponent. Maybe @gaearon or @acdlite can clarify how they'd like to handle this in Fiber.

[acdlite]

Do we need a utility? Only should have to check it once

[aweary]

@acdlite nope, shouldn't need to unless you wanted to dedupe between renderers, but probably not a big deal. I pinged mainly to verify that's where you guys would like the validation to occur 😄

[misoguy]

@aweary Thanks for the detailed comments! It was easy for me to understand and I have added the implementation as guided. :) Not sure if the warning message is good enough though.

[acdlite]

Make sure it only warns once so we don't spam the user with multiple warnings, and add a test for this behavior.

[misoguy]

Just to be clear, does this mean if the app has two different pure components, the warning should only show up once? Or does it mean to only warn once for the same pure component used numerous times in different places?
I'm guessing it's the latter but asking just to be sure :)

[acdlite]

Maybe include the suggestion to use React.Component instead? Slightly more actionable.

[acdlite]

Make sure you add the warning in Fiber, too. Then run ./scripts/fiber/record-tests.

[misoguy]

@acdlite Thanks for the comments! I'll get on it :)
BTW, I forgot to mention that by implementing this warning, it breaks the test below

  it('can override shouldComponentUpdate', () => {
    var renders = 0;
    class Component extends React.PureComponent {
      render() {
        renders++;
        return <div />;
      }
      shouldComponentUpdate() {
        return true;
      }
    }
    var container = document.createElement('div');
    ReactDOM.render(<Component />, container);
    ReactDOM.render(<Component />, container);
    expect(renders).toBe(2);
  });

How should this test be modified?

[aweary]

@misoguy we need to adjust the test to expect a warning as well. The test breaks because the test suite will fail if an expected warning is logged. You can test warnings by spying on console.error

here's an example

[misoguy]

@aweary I have adjusted the original test to spy on console.error and the test passes now, but it seems to me that the new test I have added is not really needed if its checked on the original test. Thoughts?

[misoguy]

@acdlite I have added the warning in Fiber too, and ran ./scripts/fiber/record-tests as requested. However, when adding this to fiber I have noticed a strange behavior and have a question to ask.
I had to make the test to render the pure component twice like this to pass the condition type.prototype.isPureComponent here.
I'm not sure if this an intended behavior or some kind of a bug?

[misoguy]

@aweary @acdlite I'm not trying to be pushy here but I'm going on my honeymoon starting next saturday and will not be able to track this issue for about 2 weeks. I'd like to finish up with what I started before I go :) It'd be nice to get a feedback on how I can improve this PR to get merged. Thanks!

[aweary]

@misoguy sorry for the delay here!

I had to make the test to render the pure component twice like this to pass the condition type.prototype.isPureComponent here.
I'm not sure if this an intended behavior or some kind of a bug?
View changes

I believe this was because the warning wasn't being issued until the component needed to update. I went ahead and moved it down to checkClassInstance so that the warning is emitted when the class is instantiated, not when it updates (if ever). I think we can merge once CI passes 👍 thanks for your work on this!

[misoguy]

@aweary Thank you for the comment and the added commit. It's great to know that my PR can be merged now :)

out of date

[gaearon]

I take it it’s no longer WIP? 😄

[gaearon]

What if somebody uses PureComponent on a base class, and then wants to override shouldComponentUpdate just for one subclass without changing the whole hierarchy? We don’t care about inheritance that much, but it could be annoying.

[aweary]

A work in completion 😄 btw @misoguy thanks again for doing this, great work!

What if somebody uses PureComponent on a base class, and then wants to override shouldComponentUpdate just for one subclass without changing the whole hierarchy? We don’t care about inheritance that much, but it could be annoying.

I would recommend they use Component for the base class with a shallowEqual check in shouldComponentUpdate. If future optimizations for PureComponent rely on the contract that it only updates for prop/state changes, violating that in an extending class seems like it could cause problems?

[gaearon]

Yea fair enough.

[misoguy]

@aweary Thank you for merging this PR. It's great to see my first PR to react get merged :)
@gaearon Thank you for changing the title of this PR. 👍