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

Add better guard for nested renderings. #2277

Closed
slorber opened this issue Sep 30, 2014 · 4 comments
Closed

Add better guard for nested renderings. #2277

slorber opened this issue Sep 30, 2014 · 4 comments

Comments

@slorber
Copy link
Contributor

slorber commented Sep 30, 2014

This issue demonstrate the problem I had:
http://jsfiddle.net/kb3gN/6069/

As you can see, both cases are not handled the same way.

Warning: unmountComponentAtNode(): Render methods should be a pure function of props and state; triggering nested component updates from render is not allowed. If necessary, trigger nested updates in componentDidUpdate. 10333102_793476900703299_1710860803_n.js:20226
Warning: _renderNewRootComponent(): Render methods should be a pure function of props and state; triggering nested component updates from render is not allowed. If necessary, trigger nested updates in componentDidUpdate. 10333102_793476900703299_1710860803_n.js:20226
Uncaught Error: Invariant Violation: unmountComponent(): Can only unmount a mounted component. 

VS

Uncaught Error: Invariant Violation: replaceState(...): Cannot update during an existing state transition (such as within `render`). This could potentially cause an infinite loop so it is forbidden. 10333102_793476900703299_1710860803_n.js:18689invariant 10333102_793476900703299_1710860803_n.js:18689validateLifeCycleOnReplaceState 10333102_793476900703299_1710860803_n.js:6226ReactCompositeComponentMixin.replaceState 10333102_793476900703299_1710860803_n.js:6645ReactCompositeComponentMixin.setState 10333102_793476900703299_1710860803_n.js:6626React.createClass.render Inline JSX script:23(anonymous function) 10333102_793476900703299_1710860803_n.js:7017ReactPerf.measure 10333102_793476900703299_1710860803_n.js:12710(anonymous function) 10333102_793476900703299_1710860803_n.js:6947ReactPerf.measure 10333102_793476900703299_1710860803_n.js:12710ReactCompositeComponentMixin._performComponentUpdate 10333102_793476900703299_1710860803_n.js:6891ReactCompositeComponentMixin.performUpdateIfNecessary 10333102_793476900703299_1710860803_n.js:6831runBatchedUpdates 10333102_793476900703299_1710860803_n.js:15012Mixin.perform 10333102_793476900703299_1710860803_n.js:16875Mixin.perform 10333102_793476900703299_1710860803_n.js:16875mixInto.perform 10333102_793476900703299_1710860803_n.js:14958(anonymous function) 10333102_793476900703299_1710860803_n.js:15036ReactPerf.measure 10333102_793476900703299_1710860803_n.js:12710Mixin.closeAll 10333102_793476900703299_1710860803_n.js:16948Mixin.perform 10333102_793476900703299_1710860803_n.js:16889ReactDefaultBatchingStrategy.batchedUpdates 10333102_793476900703299_1710860803_n.js:9162batchedUpdates 10333102_793476900703299_1710860803_n.js:14973ReactEventListener.dispatchEvent 10333102_793476900703299_1710860803_n.js:10663

Actually I encountered the 1st case, and it was quite difficult to see where the problem was in my real life application because there was only a warning, and the message were not very helpful. Is it possible to handle this consistantly or at least include the component name if available?

@cigzigwon
Copy link

Your code follows bad practices, first try declaring the initial state. Second, you don't need to re-render the component like this. Changing the state will re-render and you are doing it twice. Only render once initially... and your function to do that does nothing with the name parameter. Try looking at more examples to write better code.

@slorber
Copy link
Contributor Author

slorber commented Oct 2, 2014

@cigzigwon thanks but I know it is bad practice. This is not the question.

We have an internal framework heavily inspired by Om's concepts, which maintains all state outside of React and always rerenders from the top, injecting a global application state. You can read more here: http://stackoverflow.com/questions/25791034/om-but-in-javascript/25806145

This JsFiddle is just an oversimplification of the problem. Actually what happened in our application is that someone triggered an application event during the render phase. And this event triggered itself a render inside the render.

From the beginning I suspected someone did something like that. That's not the point of this issue.
This issue is about making the error message more consistant so that it always detects the potential infinite loop when the rendering is triggered from the outside.
It is also to make it clearer which component can be the cause of the problem (is: which component triggered the nested rendering).

In my case, I had to put a breakpoint on the React's warning statement to be able to find the appropriate stack entry and the concerned component. It would be easier to have an error instead of a warning, and to put the displayName of the component in the error.

Anyway the warning is not appropriate because anyway there is another error that happens a little bit later with an obscure error message. It would be better to fail fast consistently in both exposed cases.

@cigzigwon
Copy link

Ok, I get the gist of what you are saying, someone else also wanted some sort of protection from the infinite loop mistake. But yes, I'm referring to standalone React. I have successfully used other libs internally to react and it works great.

Sounds like an issue that code review/pair programming techniques can solve within your organization.

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2017

I verified we now print sensible messages in both cases. They lack deduplication but I filed #11081 for this.

@gaearon gaearon closed this as completed Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants