-
Notifications
You must be signed in to change notification settings - Fork 708
Add ViteApp and Npm Package Manager #12283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Remove the dependency on Community Toolkit in the py template. Contributes to dotnet#12199
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12283Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12283" |
|
cc @aaronpowell - I plan on moving the Npm installation and ViteApp from CommunityToolkit into dotnet/aspire so our template can use them without depending on an external package. |
Implement default dockerfile builder. Code clean up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves the AddViteApp and WithNpmPackageManager functionality from the CommunityToolkit to the core Aspire repository, removing the dependency on the Community Toolkit in the Python starter template. The implementation introduces a resource-based architecture for package installation using npm, with support for configurable package managers through annotations.
Key Changes:
- Added
ViteAppResourceandNpmInstallerResourcetypes to represent Vite applications and npm package installers as first-class resources - Implemented
JavaScriptPackageManagerAnnotationto configure package manager commands (install, run, build) - Added
WithNpmPackageManagerextension method that creates installer resources and establishes parent-child relationships with wait dependencies - Updated the Python starter template to use the new built-in methods instead of Community Toolkit packages
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.NodeJs/ViteAppResource.cs | Defines the ViteAppResource class for Vite applications |
| src/Aspire.Hosting.NodeJs/NpmInstallerResource.cs | Defines the NpmInstallerResource for npm package installation |
| src/Aspire.Hosting.NodeJs/NodeExtensions.cs | Adds AddViteApp and WithNpmPackageManager extension methods with Dockerfile generation support |
| src/Aspire.Hosting.NodeJs/JavaScriptPackageManagerAnnotation.cs | Defines annotation for configuring package manager commands |
| src/Aspire.Hosting.NodeJs/JavaScriptPackageInstallerAnnotation.cs | Defines annotation to track installer resources |
| tests/Aspire.Hosting.NodeJs.Tests/*.cs | Comprehensive test coverage for resource creation, package installation, and integration scenarios |
| src/Aspire.ProjectTemplates/templates/aspire-py-starter/13.0/apphost.cs | Updates template to use WithNpmPackageManager instead of Community Toolkit |
| playground/AspireWithJavaScript/* | Updates playground to demonstrate the new Vite app functionality |
| Aspire.slnx | Reorganizes playground folder structure in solution |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is merged, can you send a PR to deprecate the implementation in the Community Toolkit and target our aspire-13 branch
| /// <param name="useCI">When true, use <code>npm ci</code>, otherwise use <code>npm install</code> when installing packages.</param> | ||
| /// <param name="configureInstaller">Configure the npm installer resource.</param> | ||
| /// <returns>A reference to the <see cref="IResourceBuilder{T}"/>.</returns> | ||
| public static IResourceBuilder<TResource> WithNpmPackageManager<TResource>(this IResourceBuilder<TResource> resource, bool useCI = false, Action<IResourceBuilder<NpmInstallerResource>>? configureInstaller = null) where TResource : NodeAppResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete this bool and make it part of the NpmInstallerResource and a callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useCI bool? The reason I left it in is so we could use the same thing in the docker build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can leave it for now. Imagined this as a method on the callback rather than this.
|
There was an error handling pipeline event 59a60cbb-2988-4846-83a6-ebadedb8652b. |
| } | ||
| } | ||
| }, | ||
| "reactvite": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vhvb1989 this is what wil break azd, we're not deploying this resource but need to build it.
|
I'm pretty happy where this is, but if we merge it, it will break publishing the template until we have the other changes right? I'd love to get this in though... |
I can deploy the template app in this change to ACA by itself, and it works. It's just that you get an "empty" frontend docker container deployed as well until more changes from #12265 land. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well!
Description
Remove the dependency on Community Toolkit in the py template.
Move
AddViteAppandWithNpmPackageManagerimplementation from https://github.com/CommunityToolkit/Aspire/tree/main/src/CommunityToolkit.Aspire.Hosting.NodeJS.Extensions to this repo.Contributes to #12199
Checklist
<remarks />and<code />elements on your triple slash comments?