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

#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

Show file tree
Hide file tree
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
4 changes: 1 addition & 3 deletions src/renderers/shared/ReactDebugTool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,7 @@ var ReactDebugTool = {
}
},
getFlushHistory() {
if (__DEV__) {
return flushHistory;
}
return flushHistory;
},
onBeginFlush() {
if (__DEV__) {
Expand Down
32 changes: 32 additions & 0 deletions src/renderers/shared/ReactPerf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,31 @@

var ReactDebugTool = require('ReactDebugTool');
var warning = require('warning');
var alreadyWarned = false;

function roundFloat(val, base = 2) {
var n = Math.pow(10, base);
return Math.floor(val * n) / n;
}

function returnWarnIfDevFalse(returningValue = 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let’s call it warnInProduction.

if (!alreadyWarned) {
alreadyWarned = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should set this flag outside this if. Otherwise we check console every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

warning(__DEV__,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, it turns out we can’t use warning here because it gets stripped in production 😂 .
Let’s do this instead:

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.');
}
return returningValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let’s just make it return nothing, and explicitly return in the methods.

}

function getFlushHistory() {
returnWarnIfDevFalse([]);
return ReactDebugTool.getFlushHistory();
}

function getExclusive(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let’s replace these with explicit blocks:

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


var aggregatedStats = {};
var affectedIDs = {};

Expand Down Expand Up @@ -74,6 +88,8 @@ function getExclusive(flushHistory = getFlushHistory()) {
}

function getInclusive(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);

var aggregatedStats = {};
var affectedIDs = {};

Expand Down Expand Up @@ -142,6 +158,8 @@ function getInclusive(flushHistory = getFlushHistory()) {
}

function getWasted(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);

var aggregatedStats = {};
var affectedIDs = {};

Expand Down Expand Up @@ -235,6 +253,8 @@ function getWasted(flushHistory = getFlushHistory()) {
}

function getOperations(flushHistory = getFlushHistory()) {
returnWarnIfDevFalse([]);

var stats = [];
flushHistory.forEach((flush, flushIndex) => {
var {operations, treeSnapshot} = flush;
Expand All @@ -258,6 +278,8 @@ function getOperations(flushHistory = getFlushHistory()) {
}

function printExclusive(flushHistory) {
returnWarnIfDevFalse('');

var stats = getExclusive(flushHistory);
var table = stats.map(item => {
var {key, instanceCount, totalDuration} = item;
Expand All @@ -279,6 +301,8 @@ function printExclusive(flushHistory) {
}

function printInclusive(flushHistory) {
returnWarnIfDevFalse('');

var stats = getInclusive(flushHistory);
var table = stats.map(item => {
var {key, instanceCount, inclusiveRenderDuration, renderCount} = item;
Expand All @@ -293,6 +317,8 @@ function printInclusive(flushHistory) {
}

function printWasted(flushHistory) {
returnWarnIfDevFalse('');

var stats = getWasted(flushHistory);
var table = stats.map(item => {
var {key, instanceCount, inclusiveRenderDuration, renderCount} = item;
Expand All @@ -307,6 +333,8 @@ function printWasted(flushHistory) {
}

function printOperations(flushHistory) {
returnWarnIfDevFalse('');

var stats = getOperations(flushHistory);
var table = stats.map(stat => ({
'Owner > Node': stat.key,
Expand Down Expand Up @@ -344,14 +372,18 @@ function getMeasurementsSummaryMap(measurements) {
}

function start() {
returnWarnIfDevFalse();
ReactDebugTool.beginProfiling();
}

function stop() {
returnWarnIfDevFalse();
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();
return ReactDebugTool.isProfiling();
}

Expand Down