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

merged 11 commits into from
Jun 6, 2016

Conversation

sashashakun
Copy link
Contributor

@sashashakun sashashakun commented May 26, 2016 •
edited

Via #6871

Steps before PR:

  • 1. Fork the repo and create your branch from master.
  • 2. If you've added code that should be tested, add tests!
  • 3. If you've changed APIs, update the documentation. - need to investigate
  • 4. Ensure the test suite passes (grunt test).
  • 5. Make sure your code lints (grunt lint) - we've done our best to make sure these rules match our internal linting guidelines.
  • 6. If you haven't already, complete the CLA.

@sashashakun
Copy link
Contributor Author

sashashakun commented May 26, 2016 •
edited

Hmm, found some problem. I tryed to avoid copy pasting the same code at the beginnig of every method in ReactPerf and make helper returnWarnIfDevFalse for

Add warnings and early returns to all methods in ReactPerf when DEV is false

but now helper will be executed and independently of it's result, other part of method will be executed too. Need help for good solution here.

gaearon
gaearon reviewed May 26, 2016
View reviewed changes
src/renderers/shared/ReactPerf.js

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.

@ghost
Copy link

ghost commented May 26, 2016

@sashashakun updated the pull request.

@sashashakun
Copy link
Contributor Author

Also, should I squash commits before PR will be merged or github do it in right way?

@gaearon
Copy link
Collaborator

gaearon commented May 26, 2016

I’ll squash when this is ready, GitHub now lets it be done via UI.

gaearon
gaearon reviewed May 26, 2016
View reviewed changes
src/renderers/shared/ReactPerf.js
@@ -19,11 +19,28 @@ function roundFloat(val, base = 2) {
return Math.floor(val * n) / n;
}

function warnInProduction() {
if (typeof console !== 'undefined') {
console.error('ReactPerf is not supported in the production builds of React.' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: let’s start string literal from the next line.

console.error(
  'ga ga ' +
  'o la la'
);

@ghost
Copy link

ghost commented May 26, 2016

@sashashakun updated the pull request.

@ghost
Copy link

ghost commented May 26, 2016

@sashashakun updated the pull request.

@ghost
Copy link

ghost commented May 26, 2016

@sashashakun updated the pull request.

@facebook-github-bot
Copy link

@sashashakun updated the pull request.

@sashashakun
Copy link
Contributor Author

Let's also add a test similar to ReactDOMProduction in its setup so we don't break it again.
Got it. Will start ASAP.

Also, have some problem with test in travis, but don't have problems with local tests.

P.S. I am afraid that do this PR too long

@gaearon
Copy link
Collaborator

gaearon commented May 28, 2016

Also, have some problem with test in travis, but don't have problems with local tests.

Try to merge master into your branch and check if something fails locally.

P.S. I am afraid that do this PR too long

I’ve been doing the ReactPerf PR for two months so it’s not long. 😉
Take your time.

@sashashakun
Copy link
Contributor Author

Hmm, merge master branch from facebook/react to local and after that to react-perf-warnings-changes branch from sashashakun/react and that's it, no problem. Need not to forget in future update branch before new push)
Starting to write tests for this😌

@ghost
Copy link

ghost commented May 30, 2016

@sashashakun updated the pull request.

@ghost
Copy link

ghost commented Jun 4, 2016

@sashashakun updated the pull request.

gaearon
gaearon reviewed Jun 4, 2016
View reviewed changes
src/renderers/shared/__tests__/ReactPerf-test.js
spyOn(console, 'error');

expect(ReactPerf.getLastMeasurements()).toEqual([]);

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 remove the blank lines here, they don’t help much.

@ghost
Copy link

ghost commented Jun 4, 2016

@sashashakun updated the pull request.

@gaearon gaearon mentioned this pull request Jun 6, 2016
Merged
gaearon added a commit that referenced this pull request Jun 6, 2016
@gaearon
Warn that ReactPerf does not work in the production build (#6884, #6975)
f71dfbc 
Fixes #6871
gaearon added a commit that referenced this pull request Jun 6, 2016
@gaearon
Revert "Warn that ReactPerf does not work in the production build (#6884
afe4837 
, #6975)"

This reverts commit f71dfbc.

Reverting because GitHub stripped the original PR author.
@gaearon gaearon merged commit 2a46103 into facebook:master Jun 6, 2016
@gaearon gaearon mentioned this pull request Jun 6, 2016
Merged
@gaearon
Copy link
Collaborator

gaearon commented Jun 6, 2016

A little recap in case it’s confusing what happened here:

  1. I created a separate PR because I wanted to make a small change here.
  2. I pressed Squash & Merge there without realizing GitHub would erase the original author.
  3. I reverted that commit because I wanted to preserve the author.
  4. I squashed & merged this PR instead.
  5. I submitted my style change in a separate PR in Tweak ReactPerf warning message and code style #6977.

Sorry about this mess. 😓

@sashashakun Thanks for this! Sorry it took so long to review.

@zpao zpao added this to the 15-next milestone Jun 6, 2016
@sashashakun
Copy link
Contributor Author

@gaearon no problem, I understand 😌 thanks for help!👍

zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
@sashashakun @zpao
Warn that ReactPerf does not work in the production build (facebook#6884
1a2c756 
)

Fixes facebook#6871
(cherry picked from commit 2a46103)
zpao pushed a commit that referenced this pull request Jun 14, 2016
@sashashakun @zpao
Warn that ReactPerf does not work in the production build (#6884)
85fd90d 
Fixes #6871
(cherry picked from commit 2a46103)
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
@sashashakun sashashakun mentioned this pull request Dec 8, 2016
Closed
@renovate renovate bot mentioned this pull request Feb 2, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
15.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sashashakun @gaearon @facebook-github-bot @zpao