Optimize Node.js Dockerfile generation with layer caching and BuildKit mounts#12739
Optimize Node.js Dockerfile generation with layer caching and BuildKit mounts#12739
Conversation
- Copy package*.json files first for better layer caching - Add BuildKit cache mount for npm/yarn/pnpm downloads - Use --omit=dev flag for npm ci to skip dev dependencies in production - Selectively copy only node_modules and app files to runtime stage - Updated tests to reflect new optimized Dockerfile structure Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12739Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12739" |
Address feedback from @davidfowl to remove the hardcoded EXPOSE 3000 statement from the generated Dockerfiles for Node.js apps. Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
| /// </summary> | ||
| /// <param name="packageManager">The package manager executable name (npm, yarn, pnpm).</param> | ||
| /// <returns>The cache mount path for BuildKit, or null if not supported.</returns> | ||
| private static string? GetCacheMount(string packageManager) |
There was a problem hiding this comment.
@copilot - change this to be on the JavaScriptPackageManagerAnnotation instead.
There was a problem hiding this comment.
Done in 7abc3d1. Added a CacheMount property to JavaScriptPackageManagerAnnotation that stores the BuildKit cache mount path. Each package manager (npm, yarn, pnpm) now sets this when creating the annotation.
- Added CacheMount property to JavaScriptPackageManagerAnnotation - Production flags now added in WithNpm/WithYarn/WithPnpm methods - Simplified Dockerfile generation to use annotation values - Removed GetProductionInstallFlag and GetCacheMount helper functions - Updated tests to reflect new behavior Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
- Added PackageFilesPattern property to JavaScriptPackageManagerAnnotation - Dockerfile generation now uses PackageFilesPattern from annotation instead of hardcoded value - Moved production flag logic into GetDefaultNpmInstallCommand (returns both command and args) - Moved production flag logic into GetDefaultYarnInstallArgs - Moved production flag logic into GetDefaultPnpmInstallArgs - Updated tests to include PackageFilesPattern for custom package managers - Updated test expectations for pnpm to include --prod in default args Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes Docker image generation for Node.js applications by implementing better layer caching strategies. The key improvements include copying package dependency files separately before source code, using BuildKit cache mounts for package manager caches, and removing the unnecessary EXPOSE directive.
Key changes:
- Enhanced
JavaScriptPackageManagerAnnotationwith cache mount paths and package file patterns - Modified Dockerfile generation logic to separate dependency installation from source code copying for optimal layer caching
- Added package manager-specific configurations for npm, yarn, and pnpm with appropriate cache mount paths
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
JavaScriptPackageManagerAnnotation.cs |
Added CacheMount parameter and PackageFilesPatterns property to support BuildKit cache mounts and flexible package file copying |
JavaScriptHostingExtensions.cs |
Implemented Docker layer optimization by copying package files first, using cache mounts for installs, then copying source code; removed EXPOSE directive; added file system checks for yarn and pnpm |
AddNodeAppTests.cs |
Updated test expectations to match new Dockerfile generation patterns with cache mounts and optimized layer structure |
AddNodeAppTests.VerifyNodeAppWithContainerFilesGeneratesCorrectDockerfile.verified.txt |
Snapshot updated to reflect optimized Dockerfile with package files copied separately and cache mount usage |
|
This still needs more testing |
src/Aspire.Hosting.JavaScript/JavaScriptPackageManagerAnnotation.cs
Outdated
Show resolved
Hide resolved
Changed List<(string Source, string Destination)> to List<PackageFilePattern> where PackageFilePattern is a sealed record. This improves versionability by making property names part of the API contract. Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
|
@davidfowl - I think this is ready to merge now. |
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19181013909 |
…t mounts (microsoft#12739) * Initial plan * Optimize Node.js Dockerfile generation for AddNodeApp - Copy package*.json files first for better layer caching - Add BuildKit cache mount for npm/yarn/pnpm downloads - Use --omit=dev flag for npm ci to skip dev dependencies in production - Selectively copy only node_modules and app files to runtime stage - Updated tests to reflect new optimized Dockerfile structure Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> * Remove EXPOSE 3000 from generated Dockerfiles Address feedback from @davidfowl to remove the hardcoded EXPOSE 3000 statement from the generated Dockerfiles for Node.js apps. Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Move production flags and cache mounts to annotations - Added CacheMount property to JavaScriptPackageManagerAnnotation - Production flags now added in WithNpm/WithYarn/WithPnpm methods - Simplified Dockerfile generation to use annotation values - Removed GetProductionInstallFlag and GetCacheMount helper functions - Updated tests to reflect new behavior Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> * Refactor package manager configuration per reviewer feedback - Added PackageFilesPattern property to JavaScriptPackageManagerAnnotation - Dockerfile generation now uses PackageFilesPattern from annotation instead of hardcoded value - Moved production flag logic into GetDefaultNpmInstallCommand (returns both command and args) - Moved production flag logic into GetDefaultYarnInstallArgs - Moved production flag logic into GetDefaultPnpmInstallArgs - Updated tests to include PackageFilesPattern for custom package managers - Updated test expectations for pnpm to include --prod in default args Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> * Fix up dockerfile logic * Fix PackageFilesPattern * Remove optimizations from pnpm since they don't work * Tweak yarn cache mount logic * Respect new annotations in AddJavaScriptApp * Add pnpm optimizations * Replace tuple with record type for PackageFilePattern Changed List<(string Source, string Destination)> to List<PackageFilePattern> where PackageFilePattern is a sealed record. This improves versionability by making property names part of the API contract. Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Rename to CopyFilePattern * Clean up and refactor code --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Optimize Node.js Docker file generation
This PR implements best practices for Node.js Dockerfile generation to improve build performance and reduce image size.
Summary of Changes
Key Optimizations (AddNodeApp)
Better Layer Caching
package*.jsonfiles first before running installJavaScriptPackageManagerAnnotation.PackageFilesPatternsasPackageFilePatternrecordsBuildKit Cache Mounts
--mount=type=cache,target=/root/.npm--mount=type=cache,target=/usr/local/share/.cache/yarn--mount=type=cache,target=/pnpm/storeJavaScriptPackageManagerAnnotation.CacheMountpropertyArchitecture Improvements
The optimization logic has been refactored based on reviewer feedback:
JavaScriptPackageManagerAnnotationPackageFilePatternis now a sealed record type instead of a tuple for better versionabilityExample: Before vs After
Before:
After:
Expected Benefits
Recent Changes
EXPOSE 3000statement per reviewer feedbackCacheMountandPackageFilesPatternsproperties toJavaScriptPackageManagerAnnotationPackageFilesPatternsfrom using tuples to using aPackageFilePatternrecord type for better versionabilityTesting
Files Modified
src/Aspire.Hosting.JavaScript/JavaScriptHostingExtensions.cs- Core optimization logic refactoredsrc/Aspire.Hosting.JavaScript/JavaScriptPackageManagerAnnotation.cs- Added CacheMount and PackageFilesPatterns properties, introduced PackageFilePattern recordtests/Aspire.Hosting.JavaScript.Tests/AddNodeAppTests.cs- Test expectations updatedtests/Aspire.Hosting.JavaScript.Tests/AddJavaScriptAppTests.cs- Test expectations updatedtests/Aspire.Hosting.JavaScript.Tests/AddViteAppTests.cs- Test expectations updatedtests/Aspire.Hosting.JavaScript.Tests/PackageInstallationTests.cs- Test expectations updatedtests/Aspire.Hosting.JavaScript.Tests/Snapshots/*.verified.txt- Snapshot test updated✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.