-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Distribute source maps for easier debugging in Chrome's Performance tab #20186
Comments
I don't understand this issue. Why does a minified build prevent you from using the browser's profiling tools? |
You can use the profiling tools but it is hard to reason about what's happening because of the minified function names. I will give some examples of when it is useful to have the real names:
I can also answer the question "what time is used by React" which may let me use a custom implementation in performance-critical sections. I can also click on every function I don't know about and explore the code and comments there and makes sense of what is happening. |
I think I understood why you were confused. From what I wrote it seems like it isn't possible to profile the production build. I changed the title to reflect that. My proposal is for improving the experience when profiling production. |
The React Profiler API, the DevTools extension profiler, and our experimental scheduling profiler are meant to give insight into the types of things you mention. Given the complexity of the internal React implementation, the profiling tools we maintain are probably going to provide better insight than manually inspecting React the way you describe. (That kind of manual inspection is often difficult even for those of us who work in React core daily.) I wonder if providing source maps would help address the problem you mention of occasionally needing to step through React internals? |
I should have thought about source maps. I think this is the ideal solution. It's way better than my proposal. Is there a way that the Profiler API and the DevTools extension shows the time spend on user code and React code separately? Also is there a way to see which part of the React code is taking the longest? I will experiment with the scheduling profiler. This is the first time I've heard about it. |
No. These tools show the time spent in user code. This is because (a) a lot of energy has already been put into making React as fast as we can (given constraints) and (b) it's more feasible for people to performance-tune their own components and not React anyway. |
Thanks, makes sense. What do you think about the source maps idea? Are you open to discussing and implementing it? |
Discussing it seems fine! |
Adding an issue I found on the topic: Question: Building react with sourcemaps. |
This would definitely be useful for me. I actually just went looking to see if I could find sourcemaps for React (specifically we're using the minified production profiling versions of My main use case is 'occasionally digging through things while profiling', but it would also provide better context if/when errors happen deep in React's code and are reported to Bugsnag/etc. As currently the stack traces tend to be a whole lot of minified/mangled names. I would definitely want this to be as separate It looks like the places referenced in #14361 for enabling sourcemaps are: Curious, is this the only place that would need to be changed for this? If not, what would be the canonical way for enabling this (even if only to build my own versions)? |
I got this working somewhat, I updated Rollup and all of the plugins, set sourcemaps to true and Some I had to comment out though as i don't know how to add mapping support to some of these internal plugins I think I've had to give up for now though, if anyone else was looking into it that's what i've done |
@jasonwilliams Could you push your work somewhere so anyone else who's interested could look at it? |
Sure! Comparison: https://github.com/facebook/react/compare/main...jasonwilliams:addSourcMaps?expand=1 Clone it, yarn install, then run I'm sure someone who's good with Rollup could get it working fully |
|
My notes/main area of focus in #20186 (comment) was on the closure compiler. It has the ability to generate sourcemaps already, just needs to be configured to do so. Here's the diff I have locally, I had this working the other day for this part, but haven't tested again since if I modified this diff further.. so it might not be perfectly working right now.. Hopefully points people in the right direction but! It also seemed like if we switched from the java closure compiler to the JS one, it might have a nicer API to use. Unsure if/how much impact that would have on running speed/etc for it though. Click to expand/see diffdiff --git a/scripts/rollup/plugins/closure-plugin.js b/scripts/rollup/plugins/closure-plugin.js
index 258f3b6be..37c7ec52a 100644
--- a/scripts/rollup/plugins/closure-plugin.js
+++ b/scripts/rollup/plugins/closure-plugin.js
@@ -5,11 +5,21 @@ const {promisify} = require('util');
const fs = require('fs');
const tmp = require('tmp');
const writeFileAsync = promisify(fs.writeFile);
+const readFileAsync = promisify(fs.readFile);
function compile(flags) {
return new Promise((resolve, reject) => {
const closureCompiler = new ClosureCompiler(flags);
+
+ console.log(flags);
+ console.log(closureCompiler);
+ console.log(Object.keys(closureCompiler));
+
closureCompiler.run(function(exitCode, stdOut, stdErr) {
+ // console.log(stdOut);
+ // console.log(stdErr);
+ // console.log(stdErr);
+
if (!stdErr) {
resolve(stdOut);
} else {
@@ -25,11 +35,25 @@ module.exports = function closure(flags = {}) {
async renderChunk(code) {
const inputFile = tmp.fileSync();
const tempPath = inputFile.name;
- flags = Object.assign({}, flags, {js: tempPath});
+ flags = Object.assign({}, flags, {js: tempPath}, { create_source_map: tempPath + ".map"});
await writeFileAsync(tempPath, code, 'utf8');
+
+ console.log(flags);
+
const compiledCode = await compile(flags);
+
+ const compiledSourceMap = await readFileAsync(flags.create_source_map, { encoding: "utf8"});
+
+ console.log(typeof compiledCode);
+
+
+ console.log(tempPath)
+ console.log(flags.create_source_map)
+ // console.log(Object.keys(compiledCode));
+ // console.log(compiledSourceMap);
+
inputFile.removeCallback();
- return {code: compiledCode};
+ return {code: compiledCode, map: compiledSourceMap};
},
};
}; |
Switching to the Java one was done intentionally to address memory and speed problems with the JS compiler. (See #12800) |
|
That’s great research thanks @0xdevalias it’s good news that use_strict removal is built in, that’s one plugin that can go. Google closure compiler can go. It’s possible esbuild could replace both Babel and minification but that increases scope by quite a bit so I think for now I’ll keep Babel and use terser. Secondly speed isn’t my biggest concern right now and can be optimised later on |
PR added: #21946 I feel its close, but one of the plugins |
@jasonwilliams Could you confirm this is still an issue? |
Yes it’s still an issue |
@jasonwilliams Thanks for your work on that PR! It's taken me forever to get back to this, but I spent some time today going through it, leaving some rather in depth context/comments/etc, as well as researching solutions to what I believe should be the rest of the remaining issues.:
Would be awesome if we were able to get this feature finished up and merged in! |
Finding myself wanting this again this morning, trying to debug https://twitter.com/brian_d_vaughn/status/1612806202559459330 |
Update Rollup and related plugins to their most recent versions + resolve any breaking changes/deprecations/etc along the way. I made each change piece by piece, so the commit history tells a pretty good story of what was changed where/how/why. fixes #24894 For the full deepdive/context, see: - #24894 The inspiration for this came from @jasonwilliams 's PR for attempting to add sourcemap output support to React's builds: - #20186 - #21946 But I figured that it would be useful to minimise the scope of changes in that PR, and to modernise the build tooling along the way. If any of these updates rely on a node version later than `10.x`, then the following PR may have to land first, otherwise things might break on AppVeyor: - #24891 - #24892 Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
Update Rollup and related plugins to their most recent versions + resolve any breaking changes/deprecations/etc along the way. I made each change piece by piece, so the commit history tells a pretty good story of what was changed where/how/why. fixes #24894 For the full deepdive/context, see: - #24894 The inspiration for this came from @jasonwilliams 's PR for attempting to add sourcemap output support to React's builds: - #20186 - #21946 But I figured that it would be useful to minimise the scope of changes in that PR, and to modernise the build tooling along the way. If any of these updates rely on a node version later than `10.x`, then the following PR may have to land first, otherwise things might break on AppVeyor: - #24891 - #24892 Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu> DiffTrain build for [6b6d061](6b6d061) [View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)
I'm gonna do the annoying "+1" thing - this is something I'd love to see added to the React packages! (and tbh it's kind of surprising it hasn't been yet.) I've spent a fair amount of time needing to debug into the guts of React, and it would be a lot easier to do that if the prod builds actually included sourcemaps. (I know that this would increase the size of the published packages on NPM, but hopefully it would be worth it!) |
Brian asked Dan about this on Twitter: Dan's willing to look at a PR, but A) have to stick with Closure Compiler and not switch to Terser, B) there's concerns over "commitment" to keep them working vs hypothetical future build tooling changes. I'm willing to take a stab at a new PR to get sourcemaps working with Closure Compiler, and will try to take a look at that later this week. |
Good news, folks! As of a few minutes ago, I have:
Here's a screenshot of a random function from within the I've got a bunch of cleanup and additional verification to do, but I'm fairly positive I have this working as desired. Overall, I think there's 3 PRs that ought to be merged:
I'll see if I can file the cleanup PR early Monday, and hopefully we can get the Rollup 3.x PR in shortly after that. (Alternately, I could do all these in one PR. It's not that many additional changes. It's just nice to have more granular traceability to why each change was made.) update After discussing with Dan, I'm going to file the 3 separate PRs myself on Monday. Those will supersede the existing stale "generate sourcemaps" PR and the existing "Rollup 3.x" PR, and I can work on getting those pushed through ASAP. |
Okay, I just filed #26446 , which adds the sourcemap generation to React's build config! That should hopefully generate some packages via CodeSandbox CI in a few minutes, and we can now test out that the sourcemaps actually get included and do something useful :) |
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR updates the Rollup build pipeline to generate sourcemaps for production build artifacts like `react-dom.production.min.js`. It requires the Rollup v3 changes that were just merged in #26442 . Sourcemaps are currently _only_ generated for build artifacts that are _truly_ "production" - no sourcemaps will be generated for development, profiling, UMD, or `shouldStayReadable` artifacts. The generated sourcemaps contain the bundled source contents right before that chunk was minified by Closure, and _not_ the original source files like `react-reconciler/src/*`. This better reflects the actual code that is running as part of the bundle, with all the feature flags and transformations that were applied to the source files to generate that bundle. The sourcemaps _do_ still show comments and original function names, thus improving debuggability for production usage. Fixes #20186 . <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> This allows React users to actually debug a readable version of the React bundle in production scenarios. It also allows other tools like [Replay](https://replay.io) to do a better job inspecting the React source when stepping through. ## How did you test this change? - Generated numerous sourcemaps with various combinations of the React bundle selections - Viewed those sourcemaps in https://evanw.github.io/source-map-visualization/ and confirmed via the visualization that the generated mappings appear to be correct I've attached a set of production files + their sourcemaps here: [react-sourcemap-examples.zip](https://github.com/facebook/react/files/11023466/react-sourcemap-examples.zip) You can drag JS+sourcemap file pairs into https://evanw.github.io/source-map-visualization/ for viewing. Examples: - `react.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478247-e5cbdee0-83fd-4a19-bcf1-09961d3c7da4.png) - `react-dom.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478433-b5ccbf0f-8f68-42fe-9db9-9ecb97770d46.png) - `use-sync-external-store/with-selector.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478565-bc74699d-db14-4c39-9e2d-b775f8755561.png) <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. -->
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR updates the Rollup build pipeline to generate sourcemaps for production build artifacts like `react-dom.production.min.js`. It requires the Rollup v3 changes that were just merged in #26442 . Sourcemaps are currently _only_ generated for build artifacts that are _truly_ "production" - no sourcemaps will be generated for development, profiling, UMD, or `shouldStayReadable` artifacts. The generated sourcemaps contain the bundled source contents right before that chunk was minified by Closure, and _not_ the original source files like `react-reconciler/src/*`. This better reflects the actual code that is running as part of the bundle, with all the feature flags and transformations that were applied to the source files to generate that bundle. The sourcemaps _do_ still show comments and original function names, thus improving debuggability for production usage. Fixes #20186 . <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> This allows React users to actually debug a readable version of the React bundle in production scenarios. It also allows other tools like [Replay](https://replay.io) to do a better job inspecting the React source when stepping through. ## How did you test this change? - Generated numerous sourcemaps with various combinations of the React bundle selections - Viewed those sourcemaps in https://evanw.github.io/source-map-visualization/ and confirmed via the visualization that the generated mappings appear to be correct I've attached a set of production files + their sourcemaps here: [react-sourcemap-examples.zip](https://github.com/facebook/react/files/11023466/react-sourcemap-examples.zip) You can drag JS+sourcemap file pairs into https://evanw.github.io/source-map-visualization/ for viewing. Examples: - `react.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478247-e5cbdee0-83fd-4a19-bcf1-09961d3c7da4.png) - `react-dom.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478433-b5ccbf0f-8f68-42fe-9db9-9ecb97770d46.png) - `use-sync-external-store/with-selector.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478565-bc74699d-db14-4c39-9e2d-b775f8755561.png) <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> DiffTrain build for [2c8a139](2c8a139)
For those of us who were interested in sourcemap support for easier debugging/etc, just wanted to highlight 2 other issues that would improve things even more that you might like to watch/support: |
Update Rollup and related plugins to their most recent versions + resolve any breaking changes/deprecations/etc along the way. I made each change piece by piece, so the commit history tells a pretty good story of what was changed where/how/why. fixes facebook/react#24894 For the full deepdive/context, see: - facebook/react#24894 The inspiration for this came from @jasonwilliams 's PR for attempting to add sourcemap output support to React's builds: - facebook/react#20186 - facebook/react#21946 But I figured that it would be useful to minimise the scope of changes in that PR, and to modernise the build tooling along the way. If any of these updates rely on a node version later than `10.x`, then the following PR may have to land first, otherwise things might break on AppVeyor: - facebook/react#24891 - facebook/react#24892 Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu> DiffTrain build for [6b6d0617eff48860c5b4e3e79c74cbd3312cf45a](facebook/react@6b6d061) [View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR updates the Rollup build pipeline to generate sourcemaps for production build artifacts like `react-dom.production.min.js`. It requires the Rollup v3 changes that were just merged in facebook#26442 . Sourcemaps are currently _only_ generated for build artifacts that are _truly_ "production" - no sourcemaps will be generated for development, profiling, UMD, or `shouldStayReadable` artifacts. The generated sourcemaps contain the bundled source contents right before that chunk was minified by Closure, and _not_ the original source files like `react-reconciler/src/*`. This better reflects the actual code that is running as part of the bundle, with all the feature flags and transformations that were applied to the source files to generate that bundle. The sourcemaps _do_ still show comments and original function names, thus improving debuggability for production usage. Fixes facebook#20186 . <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> This allows React users to actually debug a readable version of the React bundle in production scenarios. It also allows other tools like [Replay](https://replay.io) to do a better job inspecting the React source when stepping through. ## How did you test this change? - Generated numerous sourcemaps with various combinations of the React bundle selections - Viewed those sourcemaps in https://evanw.github.io/source-map-visualization/ and confirmed via the visualization that the generated mappings appear to be correct I've attached a set of production files + their sourcemaps here: [react-sourcemap-examples.zip](https://github.com/facebook/react/files/11023466/react-sourcemap-examples.zip) You can drag JS+sourcemap file pairs into https://evanw.github.io/source-map-visualization/ for viewing. Examples: - `react.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478247-e5cbdee0-83fd-4a19-bcf1-09961d3c7da4.png) - `react-dom.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478433-b5ccbf0f-8f68-42fe-9db9-9ecb97770d46.png) - `use-sync-external-store/with-selector.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478565-bc74699d-db14-4c39-9e2d-b775f8755561.png) <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. -->
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR updates the Rollup build pipeline to generate sourcemaps for production build artifacts like `react-dom.production.min.js`. It requires the Rollup v3 changes that were just merged in #26442 . Sourcemaps are currently _only_ generated for build artifacts that are _truly_ "production" - no sourcemaps will be generated for development, profiling, UMD, or `shouldStayReadable` artifacts. The generated sourcemaps contain the bundled source contents right before that chunk was minified by Closure, and _not_ the original source files like `react-reconciler/src/*`. This better reflects the actual code that is running as part of the bundle, with all the feature flags and transformations that were applied to the source files to generate that bundle. The sourcemaps _do_ still show comments and original function names, thus improving debuggability for production usage. Fixes #20186 . <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> This allows React users to actually debug a readable version of the React bundle in production scenarios. It also allows other tools like [Replay](https://replay.io) to do a better job inspecting the React source when stepping through. ## How did you test this change? - Generated numerous sourcemaps with various combinations of the React bundle selections - Viewed those sourcemaps in https://evanw.github.io/source-map-visualization/ and confirmed via the visualization that the generated mappings appear to be correct I've attached a set of production files + their sourcemaps here: [react-sourcemap-examples.zip](https://github.com/facebook/react/files/11023466/react-sourcemap-examples.zip) You can drag JS+sourcemap file pairs into https://evanw.github.io/source-map-visualization/ for viewing. Examples: - `react.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478247-e5cbdee0-83fd-4a19-bcf1-09961d3c7da4.png) - `react-dom.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478433-b5ccbf0f-8f68-42fe-9db9-9ecb97770d46.png) - `use-sync-external-store/with-selector.production.min.js`: ![image](https://user-images.githubusercontent.com/1128784/226478565-bc74699d-db14-4c39-9e2d-b775f8755561.png) <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> DiffTrain build for commit 2c8a139.
I want to propose the addition of a new file in thereact-dom
npm package calledreact-dom.production.js
— a non-minified version ofreact-dom
production build.Edit: After some discussion(see below) it seems that distributing source maps makes more sense. Source maps will help you see the real function names and explore them. (The points below apply for source maps as well.)
Why?
There are a few ways to profile React's performance — none of them provide a low-level view of what's happening. I believe the best way to profile React is using Chrome's Performance tab using a non-minified production build. Here's why:
innerHTML
and adding and removing listeners?Disadvantages
As with every solution, there are some drawbacks to using this approach:
The text was updated successfully, but these errors were encountered: