Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplication of warning messages in nested updates (#11081) #11113

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

imanushree
Copy link

@imanushree imanushree commented Oct 5, 2017

@gaearon I have added flags for preventing duplication of both the warning messages so they print only once now.
But I noticed that the error related to 'Maximum update depth exceeded .... ' was going in infinite loop. Not sure if its an issue or I am missing something.

@reactjs-bot
Copy link

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

Thanks for the PR!

But I noticed that the error related to 'Maximum update depth exceeded .... ' was going in infinite loop.

Can you give me more details about this? How was this problem manifesting itself?

@imanushree
Copy link
Author

imanushree commented Oct 6, 2017

I wrote this component to recreate the warnings:

 class App extends Component {
  constructor() {
    super();
    this.state = {
      value: null,
    };
  }

  render() {
    this.setState({
      value: 'test',
    });

    return <div />;
  }
}

And in the process found that this error came up:

'Uncaught Error: Maximum update depth exceeded. This can happen when a component repeatedly
calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of
nested updates to prevent infinite loops.'

But the execution did not stop. It was going in an infinite loop and printing 2 variants of the same error one after another. I tested it in older versions of react and this problem is not present there. Please let me know if you want me to file an issue for this @gaearon .

Screenshot for your reference:
react-bug

@imanushree
Copy link
Author

I have created issue #11136 for this. Please take a look.

@gaearon
Copy link
Collaborator

gaearon commented Oct 6, 2017

Do you think you could add a separate test that verifies the warning is deduplicated? We typically do this by writing some test causing it to fire twice, and then asserting there has only been one warning call.

@gaearon
Copy link
Collaborator

gaearon commented Oct 6, 2017

This looks good to me. The test isn't super important tbh, as we don't test deduplication everywhere, and the code is pretty trivial.

@gaearon
Copy link
Collaborator

gaearon commented Oct 6, 2017

Thanks!

@gaearon gaearon merged commit e862966 into facebook:master Oct 6, 2017
@imanushree
Copy link
Author

@gaearon Thanks a lot Dan for merging my first PR to react ! :)

@imanushree imanushree deleted the anushree-deduplicate-warnings branch October 7, 2017 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants