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.

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

merged 11 commits into from
Jun 6, 2016

Conversation

sashashakun
Copy link
Contributor

@sashashakun sashashakun commented May 26, 2016

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

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.


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.

@@ -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.

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 added a commit that referenced this pull request Jun 6, 2016
, #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
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
zpao pushed a commit that referenced this pull request Jun 14, 2016
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants