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

Jump to bottom

#6871 - Add not dev warning for ReactPerf & remove dev checking for ReactDebugTool method #6884

Merged
merged 11 commits into from
Jun 6, 2016
Merged

#6871 - Add not dev warning for ReactPerf & remove dev checking for ReactDebugTool method #6884

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
64412a0
add not dev warning for ReactPerf & remove dev checking for ReactDebu…
sashashakun May 25, 2016
878a99f
move from warn to console.error & add explicitly returns
sashashakun May 26, 2016
2840d5e
fix linter errors
sashashakun May 26, 2016
33ae3f6
more fixes after review
sashashakun May 26, 2016
25b0810
roll back alreadyWarned variable and its changes
sashashakun May 26, 2016
ec4f40c
more fixes after review
sashashakun May 26, 2016
d868334
move from one if to two
sashashakun May 26, 2016
a8b46fb
a little fix after review
sashashakun May 28, 2016
499d822
Merge branch 'master' into react-perf-warnings-changes
sashashakun May 30, 2016
497d94b
add tests and fix bug in solution
sashashakun Jun 4, 2016
2116205
after review fixes
sashashakun Jun 4, 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
Prev Previous commit
Next Next commit
move from warn to console.error & add explicitly returns
  • Loading branch information
@sashashakun
sashashakun committed May 26, 2016
commit 878a99f27c5a8291d52de6d2e689f6e75469cf10
70 changes: 51 additions & 19 deletions src/renderers/shared/ReactPerf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,27 @@ function roundFloat(val, base = 2) {
return Math.floor(val * n) / n;
}

function returnWarnIfDevFalse(returningValue = 0) {
if (!alreadyWarned) {
alreadyWarned = true;
warning(__DEV__,
'ReactPerf is not supported in the production builds of React.' +
'To collect measurements, please use the development build of React instead.');
}
return returningValue;
function warnInProduction() {
if (typeof console !== 'undefined') {
console.error('ReactPerf is not supported in the production builds of React.' +
'To collect measurements, please use the development build of React instead.');
}
}

function getFlushHistory() {
returnWarnIfDevFalse([]);
if (!__DEV__) {
warnInProduction();
return [];
}

return ReactDebugTool.getFlushHistory();
}

function getExclusive(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);
if (!__DEV__) {
warnInProduction();
return [];
}

var aggregatedStats = {};
var affectedIDs = {};
Expand Down Expand Up @@ -88,7 +92,10 @@ function getExclusive(flushHistory = getFlushHistory()) {
}

function getInclusive(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);
if (!__DEV__) {
warnInProduction();
return [];
}

var aggregatedStats = {};
var affectedIDs = {};
Expand Down Expand Up @@ -158,7 +165,10 @@ function getInclusive(flushHistory = getFlushHistory()) {
}

function getWasted(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);
if (!__DEV__) {
warnInProduction();
return [];
}

var aggregatedStats = {};
var affectedIDs = {};
Expand Down Expand Up @@ -253,7 +263,10 @@ function getWasted(flushHistory = getFlushHistory()) {
}

function getOperations(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);
if (!__DEV__) {
warnInProduction();
return [];
}

var stats = [];
flushHistory.forEach((flush, flushIndex) => {
Expand All @@ -278,7 +291,10 @@ function getOperations(flushHistory = getFlushHistory()) {
}

function printExclusive(flushHistory) {
returnWarnIfDevFalse('');
if (!__DEV__) {
warnInProduction();
return '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to return an empty string—we can just return; here.

}

var stats = getExclusive(flushHistory);
var table = stats.map(item => {
Expand Down Expand Up @@ -317,7 +333,10 @@ function printInclusive(flushHistory) {
}

function printWasted(flushHistory) {
returnWarnIfDevFalse('');
if (!__DEV__) {
warnInProduction();
return '';
}

var stats = getWasted(flushHistory);
var table = stats.map(item => {
Expand All @@ -333,7 +352,10 @@ function printWasted(flushHistory) {
}

function printOperations(flushHistory) {
returnWarnIfDevFalse('');
if (!__DEV__) {
warnInProduction();
return '';
}

var stats = getOperations(flushHistory);
var table = stats.map(stat => ({
Expand Down Expand Up @@ -372,18 +394,28 @@ function getMeasurementsSummaryMap(measurements) {
}

function start() {
returnWarnIfDevFalse();
if (!__DEV__) {
warnInProduction();
return;
}
ReactDebugTool.beginProfiling();
}

function stop() {
returnWarnIfDevFalse();
if (!__DEV__) {
warnInProduction();
return;
}

alreadyWarned = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line looks like shouldn’t be here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see your point now. I propose we always warn just once. You can potentially leave start() and stop() in some hot spot and we don’t want to spam the logs.

Copy link
Contributor Author

@sashashakun sashashakun May 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Fix other things but need some clarification here, you mean I should leave

if (!__DEV__) {
  warnInProduction();
  return [];
}

only in start() and stop() and remove in other places?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I mean we call it everywhere because we don’t know which method gets called first, but I want the warning to only appear once, so let’s not reset the boolean variable ever.

Copy link
Contributor Author

@sashashakun sashashakun May 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ooops, understood.

ReactDebugTool.endProfiling();
}

function isRunning() {
returnWarnIfDevFalse();
if (!__DEV__) {
warnInProduction();
return;
}
return ReactDebugTool.isProfiling();
}

Expand Down