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

Perf.printWasted() doesn't work when component conditionally returns null #6885

Closed
niba opened this issue May 26, 2016 · 11 comments
Closed

Perf.printWasted() doesn't work when component conditionally returns null #6885

niba opened this issue May 26, 2016 · 11 comments
Assignees

Comments

@niba
Copy link

niba commented May 26, 2016

After upgrading to React 15.0.1 or 15.1.0 I'm not able to use printWasted from Perf tools.

React 15.0.1 throws error:
ReactDefaultPerfAnalysis.js:177 Uncaught TypeError: Cannot read property 'forEach' of undefined(…)

React 15.1.0
warning.js:44 Warning: There is an internal error in the React performance measurement code. We did not expect componentDidMount timer to stop while no timer is still in progress for another instance. Please report this as a bug in React

I discovered that error is connected with returning null. A lot of my components have something like that

render: function() {
  if (!this.props.visible) {
    return null;
  }
  // ...
}

And when I try to measure one of those components I will get above error. Removing conditions from components fixes problem.

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2016

I believe this is a duplicate of #6871.
ReactPerf doesn’t work with production build of React.
We’ll fix it not to throw, but you need to use the development build for measurements.

If you can reproduce this with the development build, please provide a minimal project showing it.
Thanks!

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2016

Oh, on a second read, I now realize it’s a different problem.
Let’s discuss 15.1.0 (we don’t have plans to make fixes to ReactPerf in 15.0.x).

@gaearon gaearon reopened this May 26, 2016
@gaearon
Copy link
Collaborator

gaearon commented May 26, 2016

warning.js:44 Warning: There is an internal error in the React performance measurement code. We did not expect componentDidMount timer to stop while no timer is still in progress for another instance. Please report this as a bug in React

This looks weird. Can you try to produce a minimal reproducing case?
We have a bunch of tests that verify returning null works, so it’s not obvious what’s going on here.

@gaearon gaearon added Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Component: Addons / Perf and removed Resolution: Duplicate labels May 26, 2016
@gaearon
Copy link
Collaborator

gaearon commented May 26, 2016

In particular, where do you run ReactPerf.start() and ReactPerf.stop()? Do you call it from the console, or do you just call it in your app?

@gaearon gaearon self-assigned this May 26, 2016
@niba
Copy link
Author

niba commented May 26, 2016

I call it from the console. When I run ReactPerf.start() and ReactPerf.stop() and during recording I don't use (don't make them visible) any components that return null when not visible everything works, there are no errors, but when I make one of those components visible, then I have errors.

I did a small test and removed all my

if (!this.props.visible) {
    return null;
}

and Perf.printWasted() started working.

I will try to prepare some code to reproduce it.

In previous version 0.14.7 I didn't have any problems with Perf tools

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2016

I will try to prepare some code to reproduce it.

Please do!

In previous version 0.14.7 I didn't have any problems with Perf tools

It was completely rewritten in 15.1.0 for a number of reasons. Some issues are expected, as the rough edges get fixed. Thanks for helping make it better.

@niba
Copy link
Author

niba commented May 28, 2016

I've discovered that issue occurs only when you use material-ui Dialog component on the other execution path, something like that

redner() {
  if (!this.props.visible) {
    return null;
  }
  return <Dialog open={this.props.visible} />
}

Here is the link to the code of Dialog component: https://github.com/callemall/material-ui/blob/master/src/Dialog/Dialog.js

On monday I will prepare full demo (they don't have a cdn version of material so I cannot make a quick demo on jsfiddle)

@gaearon
Copy link
Collaborator

gaearon commented May 28, 2016

Thank you!

@niba
Copy link
Author

niba commented May 30, 2016

Here is the repository with code: https://github.com/niba/react_perf_material_bug

I've noticed a little difference between latest version and 15.0.1. In 15.1.0 I'm able to use printWasted (I see table with results) despite the errors in logs.

@gaearon
Copy link
Collaborator

gaearon commented Jun 6, 2016

Thank you again for the repro.

I verified, and it’s a duplicate of #6842.
It is fixed in master via #6860.

I couldn’t reproduce it with master, so closing.
The fix will be out in the next release.

@gaearon gaearon closed this as completed Jun 6, 2016
@gaearon gaearon added Resolution: Duplicate and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Jun 6, 2016
@akamatala14
Copy link

Hey,
May i know the difference between calling ReacfPerf.start() and ReactPerf.stop() in console and app?

https://github.com/facebook/react/issues/6885#issuecomment-221863992

Thank you.

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

3 participants