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

React devtools 15.1.0 ctor timer error #6842

Closed
jaylaw81 opened this issue May 23, 2016 · 16 comments
Closed

React devtools 15.1.0 ctor timer error #6842

jaylaw81 opened this issue May 23, 2016 · 16 comments
Assignees

Comments

@jaylaw81
Copy link

bug

Warning displayed in console about internal error of React performance measurement code.

import React from 'react';
import { render } from 'react-dom';
import Perf from 'react-addons-perf';
import { browserHistory, Router } from 'react-router';

import routes from './routes';
import persist from 'altFlux/persist';

persist('docs');

const target = document.getElementById('app');

Perf.start();

render(
  <Router
    routes={routes}
    history={browserHistory}
  />,
  target,
  () => {
    Perf.stop();
    Perf.printWasted();
  }
);

screen shot 2016-05-23 at 9 21 08 am

15.1.0
Google Chrome

@gaearon
Copy link
Collaborator

gaearon commented May 23, 2016

Can you please provide a complete runnable example that reproduces this?

@jaylaw81
Copy link
Author

jaylaw81 commented May 23, 2016

@gaearon https://github.com/jaylaw81/sdp-bugfix

private repo.

npm install
npm start
http://localhost:3000/docs/Button > errors in console

@gaearon
Copy link
Collaborator

gaearon commented May 23, 2016

Thanks, this makes sense. For the context, I forgot about the portal use case where people ReactDOM.render again right inside a lifecycle method, thus making timers stack. We should probably keep the warning but push any timers running when a flush starts onto the stack, and pop them when the flush ends.

gaearon added a commit that referenced this issue May 25, 2016
* Don't count the time inside flushes towards lifecycle hooks

Fixes #6842.

We keep the existing behavior of testing for matching `onBeginLifeCycleTimer`/`onEndLifeCycleTimer` calls, but we push the current timer onto the stack if we enter a flush.
This solves an issue with portals which cause updates while a lifecycle timer is already running.

I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table.

* Fix up the comment
zpao pushed a commit to zpao/react that referenced this issue Jun 8, 2016
…#6860)

* Don't count the time inside flushes towards lifecycle hooks

Fixes facebook#6842.

We keep the existing behavior of testing for matching `onBeginLifeCycleTimer`/`onEndLifeCycleTimer` calls, but we push the current timer onto the stack if we enter a flush.
This solves an issue with portals which cause updates while a lifecycle timer is already running.

I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table.

* Fix up the comment

(cherry picked from commit 8d7161e)
zpao pushed a commit that referenced this issue Jun 14, 2016
* Don't count the time inside flushes towards lifecycle hooks

Fixes #6842.

We keep the existing behavior of testing for matching `onBeginLifeCycleTimer`/`onEndLifeCycleTimer` calls, but we push the current timer onto the stack if we enter a flush.
This solves an issue with portals which cause updates while a lifecycle timer is already running.

I chose to subtract the time spent in the flush from the time counted towards the lifecycle method because it would artificially inflate the “total” time of the component due to all the components inside the portal, so it would skew the exclusive table.

* Fix up the comment

(cherry picked from commit 8d7161e)
@sassanh
Copy link
Contributor

sassanh commented Jul 18, 2016

Is it considered fixed? I'm using 15.2.1.

https://www.dropbox.com/s/dqcwnukwokwubsf/Screenshot%202016-07-18%2022.57.18.png?dl=0

This warning repeated 484 times on console and then:

https://www.dropbox.com/s/3ip8vqros8biz8b/Screenshot%202016-07-18%2022.58.08.png?dl=0

this error repeated 484 times on console and page failed to render.

@sassanh
Copy link
Contributor

sassanh commented Jul 18, 2016

The problem was that I was using an undefined variable but the error/warning messages were completely misleading. After defining the variable it works without any warnings nor errors.

@gaearon
Copy link
Collaborator

gaearon commented Jul 18, 2016

Can you please provide a sample reproducing this?

@sassanh
Copy link
Contributor

sassanh commented Jul 18, 2016

Thanks for quick reply. I need to create one as the code is not open source, I'll do it asap.

@gaearon
Copy link
Collaborator

gaearon commented Jul 18, 2016

Thanks.

I’ll reopen so this is on my our radar. To be clear, this is likely not a bug in React (the actual error probably was in the component, and it got React into an inconsistent state). But we should recover from such errors, or at least not to spam you with warnings.

@gaearon gaearon reopened this Jul 18, 2016
@sassanh
Copy link
Contributor

sassanh commented Jul 18, 2016

Yeah I understand it's not a "bug" in react that produced the problem but as you said I think it could have handled better so that the developer could find the issue easier. Spamming warnings/errors and making the tab unresponsive (I couldn't even refresh or close the chrome tab and I had to kill it.) isn't nice.

@gaearon
Copy link
Collaborator

gaearon commented Jul 18, 2016

Yea, totally. At the very least we should make the warning warn at most once. This is enough for us to catch issues in tests but solves the spamming problem. The relevant code is in ReactDebugTool. Would you like to send a PR to fix it? You could keep a bool hasWarned and use that flag to decide whether to log warning or ignore it silently.

@sassanh
Copy link
Contributor

sassanh commented Jul 18, 2016

Sure, I try to manage to do it at the weekend.

@sassanh
Copy link
Contributor

sassanh commented Jul 19, 2016

Even though #7299 manages to reduce warnings, we still have problem with 484 errors and chrome tab getting unresponsive for around a minute (user can only kill it).

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2016

What errors are you referring to?

@sassanh
Copy link
Contributor

sassanh commented Jul 19, 2016

Look at this:
https://www.dropbox.com/s/3ip8vqros8biz8b/Screenshot%202016-07-18%2022.58.08.png?dl=0

I had it in my first comment too.

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2016

Those errors appear unrelated to the issue in this thread. In general, we don’t currently support gracefully recovering from errors thrown in components. This may come later (#2461) but it’s not there right now. So yes, you’ll see many errors thrown. We recommend scrolling up to the first one and fixing it (it usually causes all the others).

@sassanh
Copy link
Contributor

sassanh commented Jul 19, 2016

I see, thanks for info. So I guess we can close this.

@gaearon gaearon closed this as completed Jul 19, 2016
This issue was closed.
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