Refactor WithNpmPackageManager to WithNpm with install parameter #12325
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor WithNpmPackageManager to WithNpm with updated signature
Refactor WithNpmPackageManager to WithNpm with autoInstall parameter
Oct 23, 2025
Copilot finished work on behalf of
eerhardt
October 23, 2025 19:51
Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
Copilot
AI
changed the title
Refactor WithNpmPackageManager to WithNpm with autoInstall parameter
Refactor WithNpmPackageManager to WithNpm with install parameter
Oct 23, 2025
Copilot finished work on behalf of
eerhardt
October 23, 2025 22:21
|
I think |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12325Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12325" |
…th-npm-package-manager
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the npm package manager integration by renaming WithNpmPackageManager to WithNpm and introducing an install parameter that provides more granular control over package installation. The default behavior changes from automatically installing packages to requiring explicit opt-in via install: true.
Key Changes:
- Renamed extension method from
WithNpmPackageManagertoWithNpmwith newinstallparameter (defaults tofalse) - Split monolithic
JavaScriptPackageManagerAnnotationinto three focused annotations:JavaScriptInstallCommandAnnotation,JavaScriptRunCommandAnnotation, andJavaScriptBuildCommandAnnotation - Removed
useCIparameter as the installer now always usesnpm install
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.NodeJs/NodeExtensions.cs | Renamed method, implemented conditional installer creation, refactored to use new annotation types |
| src/Aspire.Hosting.NodeJs/JavaScriptPackageManagerAnnotation.cs | Split single annotation into three specialized annotation classes |
| tests/Aspire.Hosting.NodeJs.Tests/ResourceCreationTests.cs | Updated test names and calls, removed duplicate tests, added new test for install: false behavior |
| tests/Aspire.Hosting.NodeJs.Tests/PackageInstallationTests.cs | Updated tests to use new API, removed CI-specific test scenarios |
| tests/Aspire.Hosting.NodeJs.Tests/IntegrationTests.cs | Updated to use WithNpm(install: true) |
| tests/Aspire.Hosting.NodeJs.Tests/AddViteAppTests.cs | Updated to use WithNpm(install: true) |
| playground/AspireWithJavaScript/AspireJavaScript.AppHost/AppHost.cs | Updated all playground examples to use WithNpm(install: true) |
| src/Aspire.ProjectTemplates/templates/aspire-py-starter/13.0/apphost.cs | Updated template to use WithNpm(install: true) |
src/Aspire.Hosting.NodeJs/JavaScriptPackageManagerAnnotation.cs
Outdated
Show resolved
Hide resolved
…th-npm-package-manager
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
eerhardt
reviewed
Oct 24, 2025
eerhardt
reviewed
Oct 24, 2025
… WithInstallCommand, WithBuildCommand) Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
Copilot finished work on behalf of
eerhardt
October 24, 2025 19:54
eerhardt
reviewed
Oct 24, 2025
davidfowl
reviewed
Oct 25, 2025
davidfowl
approved these changes
Oct 26, 2025
radical
pushed a commit
that referenced
this pull request
Oct 29, 2025
) ## Refactor WithNpmPackageManager to WithNpm with install parameter ### Changes completed: - [x] Update NodeExtensions.cs to rename `WithNpmPackageManager` to `WithNpm` - [x] Change parameter name from `autoInstall` to `install` with default value `false` - [x] Update XML documentation to reflect the new parameter name and default - [x] Update example in XML documentation to use `.WithNpm(install: true)` - [x] Update all test usages to explicitly use `install: true` when installer should be created - [x] Update test for `install: false` behavior (renamed test) - [x] Update playground examples to use `.WithNpm(install: true)` - [x] Update template file in aspire-py-starter to use `.WithNpm(install: true)` - [x] Add comprehensive unit tests for new public APIs: - `WithYarn` - tests for both install=true and install=false - `WithPnpm` - tests for both install=true and install=false - `WithInstallCommand` - tests custom install command with custom args - `WithBuildCommand` - tests custom build command - Override scenarios for both install and build commands - [x] Fix existing tests to match new installer naming convention (`{name}-installer` instead of `{name}-npm-install`) - [x] Run tests to verify all changes work correctly (47 tests passed) ### Summary: Successfully added comprehensive unit tests for all new public APIs introduced in the refactoring. The tests verify: - Package manager annotations are correctly set for each package manager (npm, yarn, pnpm) - Installer resources are created/not created based on the `install` parameter - Custom install and build commands can be set using `WithInstallCommand` and `WithBuildCommand` - Existing commands can be overridden - All tests pass (47/47) * Refactor WithNpmPackageManager to WithNpm with autoInstall parameter Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> * Apply feedback: Rename autoInstall to install with default false Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> * WIP - refactor JS * First round of refactoring * Refactor and add yarn, pnmp, and WithInstall/BuildCommand APIs * Fix build * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add comprehensive unit tests for new public APIs (WithYarn, WithPnpm, WithInstallCommand, WithBuildCommand) Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com> * Update AspireWithJavaScript playground app * Add more tests * Update comments. * Apply suggestion from @eerhardt * Respond to PR feedback --------- 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: Eric Erhardt <eric.erhardt@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor WithNpmPackageManager to WithNpm with install parameter
Changes completed:
WithNpmPackageManagertoWithNpmautoInstalltoinstallwith default valuefalse.WithNpm(install: true)install: truewhen installer should be createdinstall: falsebehavior (renamed test).WithNpm(install: true).WithNpm(install: true)WithYarn- tests for both install=true and install=falseWithPnpm- tests for both install=true and install=falseWithInstallCommand- tests custom install command with custom argsWithBuildCommand- tests custom build command{name}-installerinstead of{name}-npm-install)Summary:
Successfully added comprehensive unit tests for all new public APIs introduced in the refactoring. The tests verify:
installparameterWithInstallCommandandWithBuildCommand✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.