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

Support production profiling with React Developer Tools #7737

Merged
merged 12 commits into from Oct 3, 2019

Conversation

@JacobMGEvans
Copy link
Contributor

commented Sep 26, 2019

Closes #7734.

Some of the process I went through to test the changes include the following:

  • I ran yarn test

Screen Shot 2019-09-25 at 8 32 10 PM

  • After I ran yarn build

Screen Shot 2019-09-25 at 9 52 15 PM

  • Following this, I ran yarn create-react-app my-app built a small (very small) app
    to test the react devtools profiler and source code

Screen Shot 2019-09-25 at 10 48 45 PM

Screen Shot 2019-09-25 at 10 49 06 PM

**Source Code ClassName:**

Screen Shot 2019-09-25 at 11 11 16 PM

Screen Shot 2019-09-25 at 10 51 13 PM

@facebook-github-bot

This comment has been minimized.

Copy link

commented Sep 26, 2019

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Sep 26, 2019

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@JacobMGEvans JacobMGEvans marked this pull request as ready for review Sep 26, 2019
@ianschmitz

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2019

Looks like this needs to rebased on master.

Scratch that. Target branch is wrong that's why it's giving a strange diff.

@ianschmitz ianschmitz added this to the 3.2 milestone Sep 26, 2019
@ianschmitz ianschmitz changed the title Feature profile issue#7734 Support production profiling with React Developer Tools Sep 26, 2019
@ianschmitz ianschmitz changed the base branch from feature/templates to master Sep 26, 2019
Copy link
Collaborator

left a comment

Can you revert yarn.lock.cached please?

@JacobMGEvans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

Do you mean to before I changed it and caused the merge conflict?

@ianschmitz

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2019

Do you mean to before I changed it and caused the merge conflict?

If you revert it to whats in master then we shouldn't see any changes for that file, just the webpack config should show a diff.

- git checkout origin/master -- packages/create-react-app/yarn.lock.cached
@JacobMGEvans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

Do you mean to before I changed it and caused the merge conflict?

If you revert it to whats in master then we shouldn't see any changes for that file, just the webpack config should show a diff.

I was able to revert the file to the master branch yarn.lock.cache to the previous state on commit
e0719e4c5f00d2870ae16a2b57a00c66941ea429

@JacobMGEvans JacobMGEvans requested a review from ianschmitz Sep 26, 2019
@kentcdodds

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

This is great 💯

@iansu iansu self-assigned this Sep 26, 2019
- I clarified the comment and specified the use case
- Changed the environment check to check for the specific true rather than
the assumed primitive value as before.
@JacobMGEvans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

What about yarn profile or yarn build --profile? Not sure I'm a fan of using an env variable here.

@gaearon what could be some potential pitfalls or anything else you can think of, for not doing it this way? Just curious and trying to understand where you are coming from on this 😄

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

This is slick 👍

I also think yarn build --profile (or similar, command flag) would be preferable to an ENV var.

@JacobMGEvans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

This is slick 👍

I also think yarn build --profile (or similar, command flag) would be preferable to an ENV var.

I will work on figuring out how to accomplish it this way then.

I appreciate your input and suggestions @gaearon @bvaughn

@kentcdodds

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

I think you should be able to get it with process.env.argv.includes('--profile')

@JacobMGEvans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

I think you should be able to get it with process.env.argv.includes('--profile')

Thank you very much for your help @kentcdodds 🎉 😃

- Per suggestion --profile flag used instead of env variable PROFILE_APP
Copy link
Collaborator

left a comment

Looks good!

Let's add some documentation for this. We can probably add it to https://create-react-app.dev/docs/available-scripts for now. We can probably take some of the language from here:https://github.com/facebook/create-react-app/blob/cbad256a4aacfc3084be7ccf91aad87899c63564/docusaurus/docs/available-scripts.md

Maybe something like:

... production build section for more information.

React DOM automatically supports profiling in development mode for v16.5+, but since profiling adds some small additional overhead it is opt-in for production mode. You can opt-in by using the --profile flag. Use npm run build -- --profile or yarn build --profile to enable profiling in production mode. You can read more about profiling using the React DevTools here: https://reactjs.org/docs/optimizing-performance.html#profiling-components-with-the-devtools-profiler

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2019

Once this lands, let me know and I can add it to https://fb.me/react-profiling as well.

@JacobMGEvans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

@ianschmitz I added what you suggested for potential documentation to the section you mentioned

React DOM automatically supports profiling in development mode for v16.5+, but since profiling adds some small additional overhead it is opt-in for production mode. You can opt-in by using the --profile flag. Use npm run build -- --profile or yarn build --profile to enable profiling in production mode. You can read more about profiling using the React DevTools here: https://reactjs.org/docs/optimizing-performance.html#profiling-components-with-the-devtools-profiler

If anyone has other suggestions or ideas I am more than happy to proceed with further changes or discuss further changes.

Thank you for the help and suggestions so far everyone! 😄

Copy link
Contributor

left a comment

Super 👍

@iansu iansu modified the milestones: 3.2, 3.1.3 Sep 30, 2019
docusaurus/docs/available-scripts.md Outdated Show resolved Hide resolved
@ianschmitz ianschmitz modified the milestones: 3.2, 3.3 Oct 1, 2019
- Added a brief summary of profiling in available scripts section.
The summary references the production-build document. Which is the
file I moved the new documentation into under a new Header for production support.
@ianschmitz ianschmitz modified the milestones: 3.3, 3.2 Oct 2, 2019
iansu added 2 commits Oct 3, 2019
@iansu
iansu approved these changes Oct 3, 2019
@iansu iansu merged commit 88cf8cd into facebook:master Oct 3, 2019
14 of 25 checks passed
14 of 25 checks passed
facebook.create-react-app Build #20191003.1 failed
Details
facebook.create-react-app (Behavior WindowsNode10) Behavior WindowsNode10 failed
Details
facebook.create-react-app (Behavior WindowsNode8) Behavior WindowsNode8 failed
Details
facebook.create-react-app (Installs WindowsNode10) Installs WindowsNode10 failed
Details
facebook.create-react-app (Installs WindowsNode8) Installs WindowsNode8 failed
Details
facebook.create-react-app (Kitchensink WindowsNode10) Kitchensink WindowsNode10 failed
Details
facebook.create-react-app (Kitchensink WindowsNode8) Kitchensink WindowsNode8 failed
Details
facebook.create-react-app (KitchensinkEject WindowsNode10) KitchensinkEject WindowsNode10 failed
Details
facebook.create-react-app (KitchensinkEject WindowsNode8) KitchensinkEject WindowsNode8 failed
Details
facebook.create-react-app (Simple WindowsNode10) Simple WindowsNode10 failed
Details
facebook.create-react-app (Simple WindowsNode8) Simple WindowsNode8 failed
Details
facebook.create-react-app (Behavior LinuxNode10) Behavior LinuxNode10 succeeded
Details
facebook.create-react-app (Behavior LinuxNode8) Behavior LinuxNode8 succeeded
Details
facebook.create-react-app (Behavior MacNode10) Behavior MacNode10 succeeded
Details
facebook.create-react-app (Behavior MacNode8) Behavior MacNode8 succeeded
Details
facebook.create-react-app (Installs LinuxNode10) Installs LinuxNode10 succeeded
Details
facebook.create-react-app (Installs LinuxNode8) Installs LinuxNode8 succeeded
Details
facebook.create-react-app (Kitchensink LinuxNode10) Kitchensink LinuxNode10 succeeded
Details
facebook.create-react-app (Kitchensink LinuxNode8) Kitchensink LinuxNode8 succeeded
Details
facebook.create-react-app (KitchensinkEject LinuxNode10) KitchensinkEject LinuxNode10 succeeded
Details
facebook.create-react-app (KitchensinkEject LinuxNode8) KitchensinkEject LinuxNode8 succeeded
Details
facebook.create-react-app (OldNode) OldNode succeeded
Details
facebook.create-react-app (Simple LinuxNode10) Simple LinuxNode10 succeeded
Details
facebook.create-react-app (Simple LinuxNode8) Simple LinuxNode8 succeeded
Details
netlify/create-react-app/deploy-preview Docs deploy preview succeeded
Details
@iansu

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

@kentcdodds @bvaughn This change is released now in v3.2.0.

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

Thanks a bunch!

Docs have been updated:
http://fb.me/react-profiling#create-react-app-v32

@lock lock bot locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.