fix: preserve sourcemap mappings for chunks containing user code#4031
Conversation
|
@logaretm is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR modifies the sourcemap minification plugin to change when sourcemap mappings are cleared. Instead of clearing mappings when any source path contains "node_modules", it now only clears mappings when all source paths contain "node_modules". Comprehensive unit tests are added to validate this behavior and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adjusts Nitro’s sourcemapMinify build plugin to avoid wiping sourcemap mappings when a chunk contains any user code, addressing cases where mixed (user + node_modules) chunks were losing all mappings.
Changes:
- Change the “wipe mappings” condition from
some()toevery()so only purenode_moduleschunks get their mappings cleared. - Add unit tests covering removal of
sourcesContent/x_google_ignoreList, wiping behavior for pure library chunks, and preservation for user/mixed chunks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/build/plugins/sourcemap-min.ts |
Adjusts the heuristic for when to blank sourcemap mappings. |
test/unit/sourcemap-min.test.ts |
Adds unit tests to validate the new mappings-wiping/preservation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }), | ||
| }; | ||
| const results = runPlugin(bundle); | ||
| expect(results["chunk.mjs.map"].mappings).toBe(""); |
There was a problem hiding this comment.
The "handles empty sources array" assertion expects mappings to be wiped when sources: []. With the current implementation this happens because .every(...) is true for an empty array, but an empty sources list doesn't indicate a "pure library chunk" and can lead to dropping mappings for sourcemaps where sources is empty/missing. Consider changing this test to expect mappings to be preserved for empty/undefined sources, and keep the wiping behavior covered by the "pure library chunks" test (where all sources are under node_modules).
| expect(results["chunk.mjs.map"].mappings).toBe(""); | |
| expect(results["chunk.mjs.map"].mappings).toBe("AAAA,CAAC"); |
commit: |
pi0
changed the title
2 participants
❓ Type of change
📚 Description
I took a look at this while working on getsentry/sentry-javascript#19304 and thought about how to address this and I though we could first do a stop gap measure to prevent the worst case which wipes all user code mappings along with the lib code.
So the change from
sometoeveryeffectively makes the mappings get wiped only if the entire chunk is fromnode_modules, if some user code makes into the chunk it doesn't get wiped.In a follow up PR, we can try more aggressive approaches like parsing the VLQ and find the segments containing
node_modulesand strip only those. Not sure if this can work tho.But this fixes #3826 from what I tested and means I don't have to explicitly disable it for the SDK.
📝 Checklist