fix: include esbuild build output assets in packages#13628
fix: include esbuild build output assets in packages#13628puneetdixit200 wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
📝 WalkthroughWalkthroughThe PR refactors esbuild packaging to centralize the inclusion of build artifacts and track which files are already added to prevent duplication. A new helper method adds all esbuild outputs from ChangesEsbuild packaging artifact deduplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/serverless/lib/plugins/esbuild/index.js`:
- Around line 855-872: The loop that adds include patterns currently only checks
addedFiles.has(absolutePath) so files already added from the build dir can be
duplicated when the zip entry name (path relative to service/build, e.g.
"src/locales/en.json") collides; update the logic in the functions
_addBuildFilesToZip, _package, and _packageAll to compute the zip entry name
(the relative path used in zip.file/zip.directory) for each matched file and
check addedFiles.has(entryName) in addition to addedFiles.has(absolutePath)
before adding; when adding a file, add both the absolutePath and the entryName
to addedFiles so subsequent iterations (from build artifacts or
package.patterns) properly dedupe by the zip entry name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42ec9086-e12d-4751-880f-ebafd56bb372
📒 Files selected for processing (2)
packages/serverless/lib/plugins/esbuild/index.jspackages/serverless/test/unit/lib/plugins/esbuild/index.test.js
| await Promise.all( | ||
| includesToPackage.map(async (filePath) => { | ||
| const absolutePath = path.join( | ||
| this.serverless.config.serviceDir, | ||
| filePath, | ||
| ) | ||
| if (addedFiles.has(absolutePath)) { | ||
| return | ||
| } | ||
| const stats = await stat(absolutePath) | ||
| if (stats.isDirectory()) { | ||
| zip.directory(absolutePath, filePath) | ||
| } else { | ||
| zip.file(absolutePath, { name: filePath }) | ||
| addedFiles.add(absolutePath) | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Dedup against build artifacts misses zip-entry-name collisions, allowing duplicate entries.
_addBuildFilesToZip adds build files using their path relative to the build dir as the zip entry name (e.g. src/locales/en.json) and tracks both the absolute path and that relative name in addedFiles. But this user-patterns loop only checks addedFiles.has(absolutePath), where absolutePath is rooted at serviceDir, not .serverless/build. When a user package.patterns entry matches a source file that esbuild also copied into the build dir (e.g. src/** matching src/locales/en.json — exactly the locale-asset case from #13163), the two absolute paths differ, so the file is added a second time under the same zip entry name, producing a duplicate entry in the artifact.
The relative name is already tracked precisely to catch this, but it's never consulted. The same gap exists in _packageAll at Lines 995-1009.
🐛 Proposed fix — also check the relative entry name
In _package (Lines 861-863):
- if (addedFiles.has(absolutePath)) {
- return
- }
+ if (addedFiles.has(absolutePath) || addedFiles.has(filePath)) {
+ return
+ }In _packageAll (Lines 998-1000):
- if (addedFiles.has(absolutePath)) {
- return
- }
+ if (addedFiles.has(absolutePath) || addedFiles.has(filePath)) {
+ return
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/serverless/lib/plugins/esbuild/index.js` around lines 855 - 872, The
loop that adds include patterns currently only checks
addedFiles.has(absolutePath) so files already added from the build dir can be
duplicated when the zip entry name (path relative to service/build, e.g.
"src/locales/en.json") collides; update the logic in the functions
_addBuildFilesToZip, _package, and _packageAll to compute the zip entry name
(the relative path used in zip.file/zip.directory) for each matched file and
check addedFiles.has(entryName) in addition to addedFiles.has(absolutePath)
before adding; when adding a file, add both the absolutePath and the entryName
to addedFiles so subsequent iterations (from build artifacts or
package.patterns) properly dedupe by the zip entry name.
Closes: #13163
This updates the native esbuild packaging step to include additional files emitted into
.serverless/build, such as assets copied by esbuild plugins, while still skipping generated package metadata, lockfiles,node_modules, handlers, and sourcemaps that are already added explicitly.The new unit coverage verifies both service-wide packaging and individually packaged functions include copied build output files in the final zip.
AI assistance was used while drafting this patch; I reviewed the diff and verified the results below.
Targeted checks run:
npm run test:unit -w @serverless/framework -- test/unit/lib/plugins/esbuild/index.test.js --runInBandnpx prettier -c packages/serverless/lib/plugins/esbuild/index.js packages/serverless/test/unit/lib/plugins/esbuild/index.test.jsnpx eslint packages/serverless/lib/plugins/esbuild/index.js packages/serverless/test/unit/lib/plugins/esbuild/index.test.jsgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests