Generate sourcemaps for production build artifacts #26446
Conversation
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
f389e3d
to
72029f3
Compare
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
3 times, most recently
from
c55dbf9
to
ca37e8c
Compare
|
And this PR is now green! It still needs an actual review, and also to rebased after the Rollup v3 PR is merged, but the sourcemaps work and CI is passing. |
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
ca37e8c
to
fc846c2
Compare
|
Note that this PR will supercede and invalidate past work on this such as: |
|
Will this PR also help with sourcemaps in debug builds? In the past, debugging into React source code with the VS Code debugger has been a challenge, I assume because sourcemaps were not aligned with the actual sources. |
|
@justingrant : no , I'm not trying to generate sourcemaps for dev build artifacts, because those should already be readable. Do you have an example where debugging a React dev build isn't working well? |
|
@markerikson - my experience is pretty dated from a few years ago, and was largely focused on create-react-app which handled sourcemap creation in its webpack config. Things might have improved since then. But I do remember that debugging into debug React builds seldom/never aligned with the source. If you believe it's working now, then I'm inclined to trust your judgment and assume that the problems I was seeing were caused by CRA not React's debug sourcemaps. @jasonwilliams was also looking into this so he may have more info. |
|
@gaearon you mentioned on Twitter that "the license stuff is complicated". Any specific concerns there, or other feedback on the PR? |
|
Yes. I don't fully understand what's going on there. One option is that I need to dive into this. Another is that I can pose a constraint from my perspective. This constraint is: can we reduce any manual string manipulation in this PR? Ideally there would be none. What would the approach look like if we did not have any string manipulation code? If it's due to license headers, can we inject them at a different stage (e.g. earlier), or inject them in a way that automatically adjusts the sourcemaps? What is the sequence of problems that make this code necessary? |
|
There's a couple pieces here. Rollup's own sourcemap handling normally produces a sourcemap where the "original" code is "the individual original source files", such as Because of that, what we we really want is to have a sourcemap where the "original" code is actually "the bundle contents right before Closure minified it" - in other words, the code that actually executed in the user's app. In order to make this happen, I skipped Rollup's sourcemap handling, and had Closure write a sourcemap for its minification step to disk. That contains 95% of the actual sourcemap content that we need. However, there's one more step in the Rollup pipeline after Closure. Because of that, the sourcemap from the Closure step is no longer accurate. If a particular mapping instruction pointed to a specific line segment in the output, the addition of the license text + wrapper before that in the final output throws off all the mappings. In order to align things correctly, we have to generate a sourcemap for the "possibly minified bundle -> bundle with license wrapper" step, and merge that with the "pre-minified bundle -> post-minified bundle" step. The best option I could come up with for doing that was to take the output of the license step, split it using the code to get the pieces that were added before and after, and re-run that step with a sourcemap-aware tool (the same It's maybe possible that we could move the license step before the Closure step, but I suspect that Closure would immediately strip off the license headers because those are comments. Don't know if there's somehow a Closure option that would allow us to preserve the headers. Also not sure what effect that would have on the rest of the module wrapping code that I also played with using With this approach, the bulk of the sourcemap work is in the one custom Rollup plugin there after the license plugin, and almost all the code changes are inside |
@markerikson @gaearon @jasonwilliams For historical purposes, this was also an approach I explored back in July 2022 when I was looking into fixing the things required to get sourcemap support added. From a quick search, this seems to be (at least one of the) places where I was looking at the license wrapper code, and proposed using
It includes a bit of a code exploration, some exploration of related libraries, and some proposals/diffs of how it could be used within the existing wrapper code in a 'minimally different'/low touch way, that should be simple to maintain, etc. |
|
Huh. I just tried reordering the Rollup plugins so that the license step does go before Closure and it skips the string manipulation step... and I think it's working the same way? I did a very brief search last night and think I found an indication that Closure might preserve comments with I need to do some more investigation here, but this seems promising at first glance. |
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
fc846c2
to
e485811
Compare
|
Adding the license + wrappers before the Closure step seems to work. I did have to add an extra That removes the need for the string manipulation and the extra sourcemap merging step. The sourcemaps themselves still appear to be fully accurate. Some of the RN dev bundles have increased in size, but that appears to be due to them getting formatted by Prettier more so than they were previously (function call args getting split across multiple lines, etc). No actual content changes that I can see. |
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
e485811
to
b1c8802
Compare
|
@gaearon I think this looks pretty good! Please look it over when you get a chance and let me know if you have further feedback! |
scripts/rollup/build.js
Outdated
|
|
||
| // Any other packages that should specifically _not_ have sourcemaps | ||
| const sourcemapPackageExcludes = [ | ||
| // Having `//#sourceMappingUrl` in this file breaks `ReactDevToolsHooksIntegration-test.js`, |
There was a problem hiding this comment.
Why? Can we fix the test to be more resilient instead?
There was a problem hiding this comment.
Not sure. It took me close to an hour just to figure out that having a sourcemap URL added to that bundle file was even the reason why that test was failing, and I have no idea why that was the case. Seemed to be something about imports or source parsing, maybe.
Per the comment, that file really doesn't need to have sourcemaps anyway, so it's reasonable to exclude it.
There was a problem hiding this comment.
I appreciate the time spent on this, but we generally try to understand each problem exhaustively before attempting a fix. In this case, it's important to understand why the test fails. Especially since the test has something to do with DevTools integration (which relies on parsing stacks), and might be flagging a real problem.
Even if it's not a real problem, imagine someone adding another package with similar logic, and then being confused why this logic doesn't fail tests when it's inside react-debug-tools, but does elsewhere. Then they would have to spend an hour only to find that react-debug-tools is somehow exceptional.
We do need to get to the bottom of the real cause.
There was a problem hiding this comment.
Okay, I'll dig into this further and see if I can understand what's going on.
There was a problem hiding this comment.
isStateEditable is calculated here so you might want to check if the primitive value is affected by the presence of sourcemaps. Maybe something gets borked in the stack parsing logic.
There was a problem hiding this comment.
Did some further poking at this. I think this is due to the react-debug-tools logic combined with the actual test setup in the first place.
The stack trace string, prior to being parsed, is:
Error:
at Object.apply (/home/acemarke/projects/react/scripts/jest/setupTests.js:219:29)
at Proxy.Error (/home/acemarke/projects/react/build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js:122:21)
at Object.useReducer (/home/acemarke/projects/react/build/oss-stable/react/cjs/react.production.min.js:959:21)
at MyComponent (/home/acemarke/projects/react/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js:101:13)
at renderFunction (/home/acemarke/projects/react/build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js:652:5)
at Object.inspectHooks [as inspectHooksOfFiber] (/home/acemarke/projects/react/build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js:763:12)
at Object.<anonymous> (/home/acemarke/projects/react/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js:118:32)
Note that Proxy.Error is already in the stack trace string.
react-debug-tools.production.min.js:122 is:
useInsertionEffect: function(a) {
C();
x.push({
primitive: "InsertionEffect",
stackError: Error(),
value: a
})
},Note the line stackError: Error(), and .stackError is what's getting passed to the error-stack-parser library.
If you look at scripts/jest/setupTests.js, it has this logic that overwrites global.Error with a proxy:
const ErrorProxy = new Proxy(OriginalError, {
apply(target, thisArg, argumentsList) {
const error = Reflect.apply(target, thisArg, argumentsList);
error.message = decodeErrorMessage(error.message);
return proxyErrorInstance(error);
},
construct(target, argumentsList, newTarget) {
const error = Reflect.construct(target, argumentsList, newTarget);
error.message = decodeErrorMessage(error.message);
return proxyErrorInstance(error);
},
get(target, key, receiver) {
if (key === 'captureStackTrace') {
return captureStackTrace;
}
return Reflect.get(target, key, receiver);
},
});
ErrorProxy.OriginalError = OriginalError;
global.Error = ErrorProxy;So, I think the inclusion of the sourcemap for react-debug-tools.production.min.js is causing Node to alter the stack trace related to that global.Error = ErrorProxy, which ends up breaking the parsing later.
There was a problem hiding this comment.
Sorry, I still don't quite understand. Where is the mistake? Which part is wrong here?
There was a problem hiding this comment.
@gaearon : there is no "mistake" per se.
The test setup code works as intended. The error stack parsing code works as intended.
But per my last comment, having a sourcemap for react-debug-tools.production.min.js causes Node to have different stack trace contents for the thrown error, which then causes the hook name parsing logic to fail to match what it expected to see.
We know this test worked fine when there was no sourcemap. It's an internal package, and there's no reason to have a sourcemap for this bundle.
So, we should skip it and keep the behavior the same as it was.
There was a problem hiding this comment.
We know this test worked fine when there was no sourcemap. It's an internal package, and there's no reason to have a sourcemap for this bundle.
Can confirm that this package is a part of React DevTools' backend, but for historical reasons (which I don't know) it is present as a separate package in this monorepo.
There was a problem hiding this comment.
Yeah, I did just actually happen to talk to Brian today about the fact that gets used as part of the RDT backend to parse hook names.
That said, while it's technically published ( https://www.npmjs.com/package/react-debug-tools ) , it doesn't look like it's even been published at all since the first release 5 years ago, and it seems like the only references are inside of the RDT internals. So, still no reason that I can see to justify having a sourcemap generated for that package, especially given the test situation.
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
b1c8802
to
7d71363
Compare
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
7d71363
to
5daa4ba
Compare
|
The approach in this PR looks good overall. Do you think we could ship the unminified Closure files, too, rather than using inline source maps? That would be nice for things like our sizebot diff tool which could diff the unminified files without having to go through the source maps. Relatedly, I was considering adding unminified prod files anyway (and a way to opt into those, perhaps via an |
|
@acdlite Yeah, happy to make tweaks to the output if desired. Can you clarify what you mean by "inline source maps" here? The generated sourcemaps are separate files on disk, and the |
|
Like it'd be nice to have |
|
Having #27612 merged should allow us to remove manual exclusion of |
|
Did some initial research because I genuinely didn't know the answer, and it seems as though the expectation is that if you have an "input source" listed in the sourcemap's I'll give that a shot and see how this works! |
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
8291597
to
f5825fd
Compare
markerikson
force-pushed
the
feature/build-prod-sourcemaps
branch
from
f5825fd
to
131e885
Compare
|
I've rebased the changes against I've done some testing of the generated packages locally. Looked fine in a CRA5 project. I've been struggling with Vite. I thought these changes worked fine in Vite when I last tried them a few months ago, but I think I may have been wrong. It seems like Rollup does not load library sourcemaps by default, and you have to add To be clear, I think that's a Vite/Rollup-specific question about how they load sourcemaps, rather than a problem with the build pipeline changes here. I'd still like to do some more testing to confirm how things look when the sourcemaps do get loaded into a real project, but I think the output here is (hopefully) correct. |
|
Ran a couple more checks locally. Does look like Vite (+
Spot-checked this example and I can see correct unminified production sources for Here's the JS+sourcemaps artifact from the local Vite project if anyone wants to inspect them in https://evanw.github.io/source-map-visualization/ : Vite + React + Redux app with React prod sourcemaps.zip So given that, I think this PR is ready pending @hoxyq 's updates to |
|
We've decided to keep the manual exclusion of This PR should be ready to go, I will double-check everything today and merge it. |
|
AWESOME! Thank you so much for helping push this through! Please let me know if there's any last-minute tweaks needed and I can get those done today. |
|
🥳 |
|
Thanks again for working on this @markerikson! |
|
Awesome! And thank you for helping push this across the finish line! |
<!-- 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`:  - `react-dom.production.min.js`:  - `use-sync-external-store/with-selector.production.min.js`:  <!-- 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)
|
wahooo 🥳 |
Updated React from 2983249dd to 7508dcd5c. - facebook/react#27672 - facebook/react#27132 - facebook/react#27646 - facebook/react#26446
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
shouldStayReadableartifacts.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 .
This allows React users to actually debug a readable version of the React bundle in production scenarios. It also allows other tools like Replay to do a better job inspecting the React source when stepping through.
How did you test this change?
I've attached a set of production files + their sourcemaps here:
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:react-dom.production.min.js:use-sync-external-store/with-selector.production.min.js: