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

fix(ssr): emit js sourcemaps for ssr builds #11343

Merged
merged 2 commits into from Dec 31, 2022

Conversation

thebanjomatic
Copy link
Contributor

@thebanjomatic thebanjomatic commented Dec 12, 2022

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

Additional context

Relevant rollup change that introduced this regression: rollup/rollup#4605


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

if (
bundle[file].type === 'asset' &&
!file.includes('ssr-manifest.json')
!file.includes('ssr-manifest.json') &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also .includes in the original code, but would this be better as .endsWith?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that does sound like it'd be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have updated this as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should align with this

fileName:
typeof config.build.ssrManifest === 'string'
? config.build.ssrManifest
: 'ssr-manifest.json',

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.

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.
Comment on lines 191 to 197
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make that change, I've been OOO on vacation the past few weeks

@bluwy bluwy added p4-important Violate documented behavior or significantly improves performance (priority) feat: ssr labels Dec 17, 2022
@thebanjomatic thebanjomatic requested review from bluwy and benmccann and removed request for bluwy and benmccann December 30, 2022 16:51
@thebanjomatic thebanjomatic force-pushed the fix/ssr-sourcemaps branch 2 times, most recently from 91ac43b to 4349592 Compare December 30, 2022 16:54
@benmccann
Copy link
Collaborator

I think perhaps we should hold off on this PR and merge #11430 instead, which would also fix this issue as well as others

@bluwy
Copy link
Member

bluwy commented Dec 30, 2022

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

Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. sounds fine to me

@bluwy bluwy merged commit f12a1ab into vitejs:main Dec 31, 2022
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sourcemaps are not emitted when using ssr and vite 4.0
3 participants