prevent spamming warnings related to performance measurement code #7299
Conversation
ghost
added
the
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
| @@ -52,6 +52,9 @@ var currentTimerStartTime = null; | |||
| var currentTimerNestedFlushDuration = null; | |||
| var currentTimerType = null; | |||
|
|
|||
| var beginLifeCycleTimerHasWarned = false; | |||
There was a problem hiding this comment.
I think it’s fine to have a single variable for this. If it warns once, something’s messed up already so it might warn more times.
There was a problem hiding this comment.
Just wanted to be safe about it. Thought maybe tests can produce situations that both functions generate warnings and then check the output of both. Changed it as you suggested in new commit.
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| '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; |
There was a problem hiding this comment.
This sets it regardless of whether the warning has occurred. Sorry I didn't realize this at first.
It seems like it would indeed be easier to add an if (currentTimetType) here. The warning would just get lifeCycleTimerHasWarned as an argument.
Same below.
Sorry I didn't see this at first!
There was a problem hiding this comment.
Np happens, I'm glad you found it. Just fixed it in new commit.
| currentTimerType || 'no', | ||
| (debugID === currentTimerDebugID) ? 'the same' : 'another' | ||
| ); | ||
| lifeCycleTimerHasWarned = true; |
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
I see, fixed it in a new commit.
|
This looks good to me. Have you had a chance to try this in a real project? |
|
I tested it in my project and it only warns once. |
|
Thanks! |
|
Thank you! |
) * prevent spamming warnings related to performance measurement code * minor changes in names and such * - * - (cherry picked from commit 1cc9a5d)
Preventing spamming warnings related to React performance measurement code: #6842