fix(ssr): emit js sourcemaps for ssr builds #11343
Conversation
| if ( | ||
| bundle[file].type === 'asset' && | ||
| !file.includes('ssr-manifest.json') | ||
| !file.includes('ssr-manifest.json') && |
There was a problem hiding this comment.
This was also .includes in the original code, but would this be better as .endsWith?
There was a problem hiding this comment.
yeah, that does sound like it'd be better
There was a problem hiding this comment.
Thanks, I have updated this as well.
There was a problem hiding this comment.
I think we should align with this
vite/packages/vite/src/node/ssr/ssrManifestPlugin.ts
Lines 79 to 82 in 394148e
instead since the manifest file name can be changed. It's a separate bug though so it can be handled separately if it's a lot of work.
thebanjomatic
force-pushed
the
fix/ssr-sourcemaps
branch
from
9592328
to
2944fd8
Compare
Rollup 3 switched the way sourcemaps were handled to be more consistent between the in-memory representation and what ultimately gets emitted to disk. Part of this change was moving the sourcemaps themselves to be assets within the bundle. At the same time, vite's `generateBundle` hook for the `assetPlugin` is configured to delete assets within the bundle (with the exception of "ssr-manifest.json"). Since `.js.map` files are now considered assets within the bundle at this stage, vite is removing the sourcemaps from the bundle when `build.ssr = true` and they never wind up on disk. This change adds an additional check to see if the file ends with `.js.map`, `.cjs.map`, or `.mjs.map` and it will leave these files in the bundle so that they are emitted to the outDir when the bundle is written.
thebanjomatic
force-pushed
the
fix/ssr-sourcemaps
branch
from
2944fd8
to
d1911d6
Compare
| const assetFiles = Object.keys(bundle).filter( | ||
| (file) => bundle[file].type === 'asset', | ||
| ) | ||
| for (const file of assetFiles) { | ||
| if ( | ||
| bundle[file].type === 'asset' && | ||
| !file.includes('ssr-manifest.json') | ||
| !file.endsWith('ssr-manifest.json') && | ||
| !jsSourceMapRE.test(file) |
There was a problem hiding this comment.
I think we should avoid the Object.keys and filter here as they create new objects that takes up memory. We could stick with the for in loop like before and add the sourcemap check for a tiny bit of perf.
There was a problem hiding this comment.
I will make that change, I've been OOO on vacation the past few weeks
bluwy
added
p4-important
thebanjomatic
requested review from
bluwy and
benmccann
and removed request for
bluwy and
benmccann
thebanjomatic
force-pushed
the
fix/ssr-sourcemaps
branch
2 times, most recently
from
91ac43b
to
4349592
Compare
|
I think perhaps we should hold off on this PR and merge #11430 instead, which would also fix this issue as well as others |
|
#11430 introduces a more controversial change which needs to be discussed in a meeting first. I think we should fix this soon as a patch when it's ready. |
Fix: #11344
Description
Rollup 3 switched the way sourcemaps were handled to be more consistent between the in-memory representation and what ultimately gets emitted to disk. Part of this change was moving the sourcemaps themselves to be assets within the bundle.
At the same time, vite's
generateBundlehook for theassetPluginis configured to delete assets within the bundle (with the exception of "ssr-manifest.json"). Since.js.mapfiles are now considered assets within the bundle at this stage, vite is removing the sourcemaps from the bundle whenbuild.ssr = trueand they never wind up on disk.This change adds an additional check to see if the file ends with
.js.map,.cjs.map, or.mjs.mapand it will leave these files in the bundle so that they are emitted to the outDir when the bundle is written.Additional context
Relevant rollup change that introduced this regression: rollup/rollup#4605
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).