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.

Sign up for GitHub

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

prevent spamming warnings related to performance measurement code #7299

Merged
merged 4 commits into from
Jul 19, 2016
Merged

prevent spamming warnings related to performance measurement code #7299

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2662a2d
prevent spamming warnings related to performance measurement code
sassanh Jul 18, 2016
5bfb08b
minor changes in names and such
sassanh Jul 18, 2016
2d51f65
-
sassanh Jul 18, 2016
c5d98dd
-
sassanh Jul 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 24 additions & 20 deletions src/renderers/shared/ReactDebugTool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,16 +111,18 @@ function beginLifeCycleTimer(debugID, timerType) {
if (currentFlushNesting === 0) {
return;
}
warning(
!currentTimerType || lifeCycleTimerHasWarned,
'There is an internal error in the React performance measurement code. ' +
'Did not expect %s timer to start while %s timer is still in ' +
'progress for %s instance.',
timerType,
currentTimerType || 'no',
(debugID === currentTimerDebugID) ? 'the same' : 'another'
);
lifeCycleTimerHasWarned = true;
if (!lifeCycleTimerHasWarned) {
warning(
!currentTimerType,
'There is an internal error in the React performance measurement code. ' +
'Did not expect %s timer to start while %s timer is still in ' +
'progress for %s instance.',
timerType,
currentTimerType || 'no',
(debugID === currentTimerDebugID) ? 'the same' : 'another'
);
lifeCycleTimerHasWarned = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is still incorrect.
Let me clarify: warning() doesn’t actually produce a warning unless its first argument is false.

This code is not a warning path by itself.

var isEverythingBad = false;
warning(!isEverythingBad, 'Everything is bad');

will not produce a warning. I know it’s a bit confusing but if you think of warning() as of something like assert(), it will make sense. The first argument is the condition you expect to be true.

In the current code, you set lifeCycleTimerHasWarned in any case, regardless of whether the warning was actually printed. So it gets silenced on the first check, not on the first warning.

This should work instead:

if (currentTimerType) {
  warning(
    lifeCycleTimerHasWarned,
    ...
  );
  lifeCycleTimerHasWarned = true;
}

Alternatively:

if (currentTimerType && !lifeCycleTimerHasWarned) {
  warning(
    false,
    ...
  );
  lifeCycleTimerHasWarned = true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, fixed it in a new commit.

}
currentTimerStartTime = performanceNow();
currentTimerNestedFlushDuration = 0;
currentTimerDebugID = debugID;
Expand All @@ -131,16 +133,18 @@ function endLifeCycleTimer(debugID, timerType) {
if (currentFlushNesting === 0) {
return;
}
warning(
currentTimerType === timerType || lifeCycleTimerHasWarned,
'There is an internal error in the React performance measurement code. ' +
'We did not expect %s timer to stop while %s timer is still in ' +
'progress for %s instance. Please report this as a bug in React.',
timerType,
currentTimerType || 'no',
(debugID === currentTimerDebugID) ? 'the same' : 'another'
);
lifeCycleTimerHasWarned = true;
if (!lifeCycleTimerHasWarned) {
warning(
currentTimerType === timerType,
'There is an internal error in the React performance measurement code. ' +
'We did not expect %s timer to stop while %s timer is still in ' +
'progress for %s instance. Please report this as a bug in React.',
timerType,
currentTimerType || 'no',
(debugID === currentTimerDebugID) ? 'the same' : 'another'
);
lifeCycleTimerHasWarned = true;
}
if (isProfiling) {
currentFlushMeasurements.push({
timerType,
Expand Down