Vite: Fix missing source map warning #28984
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 06b3056. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
valentinpalkovic
force-pushed
the
valentin/fix-missing-source-map-warning-second-attempt
branch
from
1e20df1
to
e6d7ac5
Compare
…/fix-missing-source-map-warning-second-attempt
also cleanup code slightly
… of github.com:storybookjs/storybook into valentin/fix-missing-source-map-warning-second-attempt
| if (!isUserCode(mod.id)) { | ||
| return; | ||
| } | ||
| mod.importedIds | ||
| .concat(mod.dynamicallyImportedIds) | ||
| .filter((name) => isUserCode(name)) | ||
| .forEach((depIdUnsafe) => { | ||
| const depId = normalize(depIdUnsafe); | ||
| if (!statsMap.has(depId)) { | ||
| statsMap.set(depId, createStatsMapModule(depId, [mod.id])); | ||
| return; | ||
| } | ||
| const m = statsMap.get(depId); | ||
| if (!m) { | ||
| return; | ||
| } | ||
| m.reasons = (m.reasons ?? []) | ||
| .concat(createReasons([mod.id])) | ||
| .filter((r) => r.moduleName !== depId); | ||
| statsMap.set(depId, m); | ||
| }); |
There was a problem hiding this comment.
might not be obvious from the diff, but this just changes the flow to use early-return, no actual logic changed.
|
@IanVS could you try out this canary and ensure it doesn't break your Turbosnap? I've done some things that should hopefully keep the same stats-file output as before, to not signal file-changes. |
JReinhold
added
the
patch:yes
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| !moduleName.match(/node_modules\//) | ||
| (moduleName && | ||
| // keep Storybook's virtual files because they import the story files, so they are essential to the module graph | ||
| Object.values(SB_VIRTUAL_FILES).includes(getOriginalVirtualModuleId(moduleName))) || |
There was a problem hiding this comment.
style: Consider using a more descriptive variable name instead of 'moduleName'
| if (!statsMap.has(depId)) { | ||
| statsMap.set(depId, createStatsMapModule(depId, [mod.id])); | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: Consider extracting this logic into a separate function for better maintainability
|
@JReinhold I tested out the canary, and TurboSnap seems to still be working correctly. 🙌 Though I can't speak for the effectiveness of the PR itself, since I don't see the referenced warnings either before or after installing the canary. |
|
Thanks for checking @IanVS! ❤️
That's okay, we've been able to reproduce it fairly easy, and this seems to solve it. |
JReinhold
changed the title
JReinhold
removed
the
patch:yes
JReinhold
deleted the
valentin/fix-missing-source-map-warning-second-attempt
branch
Closes #28567
What I did
I have added a null byte to the resolveID for virtual files. Virtual modules should be prefixed with a null byte to avoid a
false positive "missing source" warning. Context: https://github.com/vitejs/vite/pull/5587/files#diff-414e1bd440bdc6c3c213c6f9cc712d2b29b1c1662b04a9daa9c02ee6bcca9432R11-R15
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-28984-sha-29839935. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-28984-sha-29839935valentin/fix-missing-source-map-warning-second-attempt298399351725631255)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=28984Greptile Summary
This PR addresses the issue of missing source map warnings for virtual files in Vite projects by modifying the virtual file naming system and handling in Storybook's Vite builder.
SB_VIRTUAL_FILESobject invirtual-file-names.tsto centralize virtual file pathsgetResolvedVirtualModuleIdfunction to add null byte prefix to virtual module IDswebpack-stats-plugin.tsto improve handling of virtual files and maintain backwards compatibilityvue-component-meta.tsto exclude virtual modules with null byte prefix in regular expressioncode-generator-plugin.tsandexternal-globals-plugin.tsto use new virtual file naming system