[release/13.2] Handle brownfield TypeScript aspire init#15123
[release/13.2] Handle brownfield TypeScript aspire init#15123mitchdenny wants to merge 15 commits intomicrosoft:release/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15123Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15123" |
|
I think this one is important for TS, but it is also probably okay to skip if not ready. |
825ae77 to
f8fbeb3
Compare
32bcd3d to
aa94876
Compare
|
@mitchdenny any pushback on us closing this PR or retargeting against main? To have it stop showing on the list of open PRs against 13.2 |
6c0debe to
b7e5990
Compare
|
@sebastienros could I get your polyglot eyes on this? The scenario here is that someone has a pre-existing Node.js repo and they want to just do |
There was a problem hiding this comment.
Pull request overview
This PR improves TypeScript aspire init behavior for brownfield repos (e.g., Vite at repo root) by avoiding destructive overwrites and introducing a merge strategy for existing package.json/TypeScript config, along with tests validating the scenario.
Changes:
- Add a
package.jsonmerge implementation that preserves existing scripts/metadata while adding Aspire-prefixed scripts and semver-aware dependency upgrades. - Update TypeScript AppHost scaffolding/runtime to use an AppHost-specific
tsconfig.apphost.jsonand Aspire-owned script names (aspire:*). - Add unit + E2E coverage for brownfield Vite-at-root initialization and source-build Docker E2E prerequisites.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.CodeGeneration.TypeScript.Tests/TypeScriptLanguageSupportTests.cs | Adds unit tests for TypeScript scaffolding and runtime spec args. |
| tests/Aspire.Cli.Tests/Scaffolding/PackageJsonMergerTests.cs | Comprehensive tests for script conflict handling, dependency merging, engines behavior, and serialization fidelity. |
| tests/Aspire.Cli.Tests/Commands/NewCommandTests.cs | Switches config loading to AspireConfigFile API. |
| tests/Aspire.Cli.EndToEnd.Tests/TypeScriptStarterTemplateTests.cs | Updates TS starter E2E to support SourceBuild bundle/local channel scenarios. |
| tests/Aspire.Cli.EndToEnd.Tests/TypeScriptPolyglotTests.cs | Adds E2E coverage for aspire init in a brownfield Vite repo at root. |
| tests/Aspire.Cli.EndToEnd.Tests/Helpers/CliE2ETestHelpers.cs | Adds helper to prepare a local NuGet channel for SourceBuild E2E runs. |
| src/Shared/NpmVersionHelper.cs | Introduces npm-style version parsing/comparison helpers using Semver. |
| src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs | Updates scaffolding to add aspire:* scripts and emit tsconfig.apphost.json; runtime uses that config via tsx --tsconfig. |
| src/Aspire.Hosting.CodeGeneration.TypeScript/Aspire.Hosting.CodeGeneration.TypeScript.csproj | Links shared NpmVersionHelper and adds Semver dependency. |
| src/Aspire.Cli/Templating/Templates/ts-starter/tsconfig.apphost.json | Adds AppHost-specific TS config to the template. |
| src/Aspire.Cli/Templating/Templates/ts-starter/package.json | Renames scripts to aspire:* and adds convenience aliases. |
| src/Aspire.Cli/Templating/CliTemplateFactory.TypeScriptStarterTemplate.cs | Updates comments and uses AspireConfigFile for channel writing. |
| src/Aspire.Cli/Scaffolding/ScaffoldingService.cs | Merges existing package.json on write; pre-adds JS hosting package for brownfield TS. |
| src/Aspire.Cli/Scaffolding/PackageJsonMerger.cs | New merger implementing script conflict rules, dependency semver upgrades, engines.node enforcement, and deep-merge behavior. |
| src/Aspire.Cli/Aspire.Cli.csproj | Links shared NpmVersionHelper into the CLI project. |
| /// Copies locally-built NuGet packages to the workspace for SourceBuild mode. | ||
| /// Returns null for non-SourceBuild modes (CI installs packages via the PR script). | ||
| /// </summary> | ||
| /// <summary> | ||
| /// Finds the extracted bundle layout directory for SourceBuild mode. | ||
| /// The bundle provides the aspire-managed server and DCP needed for template creation. | ||
| /// Returns null for non-SourceBuild modes (CI installs the full bundle via the PR script). | ||
| /// </summary> |
There was a problem hiding this comment.
The XML doc comment above FindLocalBundlePath contains two consecutive <summary> blocks (lines 105-113), which produces invalid XML documentation (and can trigger CS1570/CS1587 warnings-as-errors). Consolidate into a single <summary> (or move one block to <remarks>).
| /// Copies locally-built NuGet packages to the workspace for SourceBuild mode. | |
| /// Returns null for non-SourceBuild modes (CI installs packages via the PR script). | |
| /// </summary> | |
| /// <summary> | |
| /// Finds the extracted bundle layout directory for SourceBuild mode. | |
| /// The bundle provides the aspire-managed server and DCP needed for template creation. | |
| /// Returns null for non-SourceBuild modes (CI installs the full bundle via the PR script). | |
| /// </summary> | |
| /// Finds the extracted bundle layout directory for SourceBuild mode. | |
| /// Returns <c>null</c> for non-SourceBuild modes. | |
| /// </summary> | |
| /// <param name="repoRoot">The repository root path.</param> | |
| /// <param name="installMode">The Docker install mode.</param> | |
| /// <returns> | |
| /// The path to the bundle layout directory, or <c>null</c> when not running in SourceBuild mode. | |
| /// </returns> |
There was a problem hiding this comment.
Fixed in 14da0ff — removed the stale <summary> block left over from the PrepareLocalChannel extraction.
| /// <summary> | ||
| /// Provides language support for TypeScript AppHosts. | ||
| /// Implements scaffolding, detection, and runtime configuration. | ||
| /// </summary> | ||
| internal sealed class TypeScriptLanguageSupport : ILanguageSupport | ||
| public sealed class TypeScriptLanguageSupport : ILanguageSupport | ||
| { |
There was a problem hiding this comment.
TypeScriptLanguageSupport was changed from internal to public, which makes it a new public API surface for the Aspire.Hosting.CodeGeneration.TypeScript package. This conflicts with the PR checklist/description stating no public API was added and is also inconsistent with other language support implementations (Go/Java/Python/Rust) which remain internal. Consider reverting it to internal and relying on the existing InternalsVisibleTo entries for tests/host instead.
There was a problem hiding this comment.
Reverted to internal in 14da0ff. This was an unintentional change from the rebase — all other language support implementations are internal and InternalsVisibleTo covers the test/host projects.
| packageJson = new JsonObject | ||
| { | ||
| ["name"] = packageName, | ||
| ["version"] = "1.0.0", |
There was a problem hiding this comment.
When scaffolding a brand-new project (package.json does not already exist), the generated package.json no longer sets "private": true even though the ts-starter template does. This can allow accidental npm publish of a scaffolded AppHost package; consider restoring private: true for new-project scaffolds.
| ["version"] = "1.0.0", | |
| ["version"] = "1.0.0", | |
| ["private"] = true, |
There was a problem hiding this comment.
Added ["private"] = true for the greenfield scaffold in 14da0ff, matching what the ts-starter template already sets. For brownfield, we intentionally don't force it — the existing project may be deliberately publishable.
|
|
||
| // NOTE: The engines.node constraint must match ESLint 10's own requirement | ||
| // (^20.19.0 || ^22.13.0 || >=24) to avoid install/runtime failures on unsupported Node versions. | ||
| packageJson["engines"] = new JsonObject | ||
| { | ||
| ["node"] = "^20.19.0 || ^22.13.0 || >=24" | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
CreatePackageJson only adds the required engines.node constraint when there is no existing package.json (i.e., packageJson is null). In the brownfield scenario (where a package.json exists but has no engines or a different Node constraint), the scaffold output won’t include the enforced Node version range, and PackageJsonMerger.MergeEngines can’t apply it. If Aspire needs to enforce the Node constraint for ESLint 10/tooling compatibility, ensure the scaffold output always includes the desired engines.node value regardless of whether an existing package.json is present.
| // NOTE: The engines.node constraint must match ESLint 10's own requirement | |
| // (^20.19.0 || ^22.13.0 || >=24) to avoid install/runtime failures on unsupported Node versions. | |
| packageJson["engines"] = new JsonObject | |
| { | |
| ["node"] = "^20.19.0 || ^22.13.0 || >=24" | |
| }; | |
| } | |
| } | |
| // NOTE: The engines.node constraint must match ESLint 10's own requirement | |
| // (^20.19.0 || ^22.13.0 || >=24) to avoid install/runtime failures on unsupported Node versions. | |
| var engines = EnsureObject(packageJson, "engines"); | |
| engines["node"] = "^20.19.0 || ^22.13.0 || >=24"; |
There was a problem hiding this comment.
Fixed in 14da0ff — moved engines.node outside the if (packageJson is null) block so it's always set in the scaffold output for both greenfield and brownfield. The CLI-side MergeEngines will then correctly apply/overwrite it.
There was a problem hiding this comment.
That change isn't part of this PR.
There was a problem hiding this comment.
This was due to the two-branch issue — we had pushed the fix to microsoft/aspire instead of mitchdenny/aspire (the actual PR head). That's been corrected: all commits are now on the right branch, and the accidental microsoft/aspire branch has been deleted. This change should now be visible in the PR diff.
| return new JsonObject(); | ||
| } | ||
|
|
||
| return JsonNode.Parse(content)?.AsObject() ?? new JsonObject(); |
There was a problem hiding this comment.
LoadExistingPackageJson can throw for malformed JSON or non-object roots (e.g., JsonNode.Parse throwing JsonException, or .AsObject() throwing InvalidOperationException when the root is an array). That would cause aspire init to fail even though the CLI has merge/fallback behavior for problematic package.json content. Consider catching parse/cast exceptions here and returning null/empty so scaffolding can proceed safely.
| return JsonNode.Parse(content)?.AsObject() ?? new JsonObject(); | |
| try | |
| { | |
| var node = JsonNode.Parse(content); | |
| if (node is JsonObject obj) | |
| { | |
| return obj; | |
| } | |
| } | |
| catch (JsonException) | |
| { | |
| // Ignore malformed JSON and fall back to an empty object so scaffolding can proceed. | |
| } | |
| catch (InvalidOperationException) | |
| { | |
| // Ignore non-object roots and fall back to an empty object so scaffolding can proceed. | |
| } | |
| return new JsonObject(); |
There was a problem hiding this comment.
Hardened in 14da0ff — LoadExistingPackageJson now catches JsonException, IOException, UnauthorizedAccessException, and InvalidOperationException, returning new JsonObject() on failure so scaffolding can proceed. Also switched from .AsObject() to as JsonObject pattern for the safe cast.
|
|
||
| private static readonly JsonSerializerOptions s_jsonOptions = new() | ||
| { | ||
| WriteIndented = true, |
There was a problem hiding this comment.
What is the standard for package.json indenting? (number of characters) Double check we match the standard and maybe explicitly specifiy indent amount.
Add a comment here saying these options have been chosen to produce a json file consistent with package.json standard.
There was a problem hiding this comment.
Good call — npm standard is 2 spaces. Fixed in 98c2c22: explicitly set IndentSize = 2 and added a comment explaining these options produce output consistent with npm init/npm install formatting conventions.
| /// <c>aspire:X</c> scripts get a convenience alias <c>X</c> pointing to <c>npm run aspire:X</c>. | ||
| /// </summary> | ||
| /// <returns>The merged package.json content as a JSON string.</returns> | ||
| internal static string Merge(string existingContent, string scaffoldContent, ILogger? logger = null) |
There was a problem hiding this comment.
No good reason — it was just convenience for unit tests. Fixed in 98c2c22: logger is now non-nullable. Tests pass NullLogger.Instance via a local MergeJson() helper.
| var existingJson = JsonNode.Parse(existingContent) as JsonObject; | ||
| var scaffoldJson = JsonNode.Parse(scaffoldContent) as JsonObject; |
There was a problem hiding this comment.
Apparently comments are not allowed in package.json, but I think we should still be safe and have options to ignore them and trailing commas.
Add test for this scenario. It's ok if comments and trailing commas are lost during the merge.
There was a problem hiding this comment.
Good point — added JsonCommentHandling.Skip and AllowTrailingCommas = true to the parse options in 98c2c22. Also added a test (ExistingJsonWithCommentsAndTrailingCommas_MergesSuccessfully) that verifies a package.json with // comments and trailing commas is handled gracefully. Comments/trailing commas are stripped in the output (standard JSON), which is fine.
| if (string.IsNullOrWhiteSpace(existingContent)) | ||
| { | ||
| return scaffoldContent; | ||
| } | ||
|
|
||
| var existingJson = JsonNode.Parse(existingContent) as JsonObject; | ||
| var scaffoldJson = JsonNode.Parse(scaffoldContent) as JsonObject; | ||
|
|
||
| if (existingJson is null || scaffoldJson is null) | ||
| { | ||
| return scaffoldContent; | ||
| } |
There was a problem hiding this comment.
What about splitting this into its own try/catch? One try/catch for parsing content, then another for merging content.
There was a problem hiding this comment.
Done in 98c2c22 — split into Phase 1 (parse inputs) and Phase 2 (merge). Each has its own try/catch with distinct log messages ("Failed to parse..." vs "Failed to merge..."). InvalidOperationException still propagates from Phase 2 since that indicates a programming error (unsupported array property in scaffold).
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger?.LogWarning(ex, "Failed to merge existing package.json, using scaffold output as-is"); |
There was a problem hiding this comment.
Fixed in 98c2c22 — both log messages now end with a period.
Not sure what happened there. I think at some point I must have jumped into VSCode and triggered a background fix or something. Fixing it all up now. Should be able to review soon. |
|
Good catch on the two branches @JamesNK — when we pushed our review-fix commits we accidentally pushed to
The PR should now show the complete diff including the review fixes. Sorry for the confusion. |
|
Replaced by #15562 — moved the head branch from the fork to the upstream repo ( |
|
Stupid AI. |
|
@JamesNK any further issues? |
JamesNK
left a comment
There was a problem hiding this comment.
Main concern is the breaking change for existing 13.2.0 TypeScript AppHost projects — the --tsconfig tsconfig.apphost.json flag in the RuntimeSpec and the --no-install addition will cause aspire run to fail for projects that were created before this PR (they only have tsconfig.json, not tsconfig.apphost.json). There's no migration or fallback logic in the run path.
src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/TypeScriptLanguageSupport.cs
Show resolved
Hide resolved
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
| } | ||
| } | ||
|
|
||
| private static JsonObject EnsureObject(JsonObject parent, string propertyName, ILogger? logger = null) |
There was a problem hiding this comment.
Fixed in cea52a0 — made EnsureObject's logger parameter non-nullable (ILogger logger instead of ILogger? logger = null), consistent with the Merge method signature. All callers now pass the logger explicitly.
| } | ||
| }; | ||
|
|
||
| var runtime = new GuestRuntime(spec, NullLogger.Instance); |
There was a problem hiding this comment.
nit: I think there should be a helper method to create GuestRuntime that all tests use. It should be created with a logger that outputs to ITestOutputHelper. Makes these tests more debuggable if they fail.
There was a problem hiding this comment.
Done in cea52a0. Added a CreateRuntime(spec?, commandResolver?) helper that uses ITestOutputHelper via LoggerFactory.Create(builder => builder.AddXunit(outputHelper)). Replaced all 19 new GuestRuntime(..., NullLogger.Instance) callsites with CreateRuntime(...). Also updated the ProcessGuestLauncher test to use the test logger.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update ts-starter template package.json build/watch scripts to use tsconfig.apphost.json - Add eslint.config.mjs scaffolding, ESLint deps, and engines constraint to CreatePackageJson - Add aspire:lint script to brownfield scaffolding - Update dependency versions to match release/13.2 baseline - Update test assertions for new dependency versions and eslint Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When aspire init is run on an existing JS/TS codebase, the scaffold RPC server returns package.json content that may not include brownfield merge logic (depending on the server package version loaded). This adds a safety net in the CLI's ScaffoldingService that deep-merges the scaffold output with the existing package.json on disk. The merge preserves all existing properties (name, version, scripts, dependencies) and only adds new properties from the scaffold. For nested objects like scripts and dependencies, existing values are never overwritten — only missing entries are added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract the package.json merge logic from ScaffoldingService into a dedicated PackageJsonMerger class with conflict-aware script handling. When scaffold scripts conflict with existing user scripts, they are added under the aspire: namespace prefix (a standard npm convention). For aspire:X scripts with no non-prefixed X equivalent, a convenience alias is added (e.g. "start": "npm run aspire:start"). This ensures all Aspire scripts are always present in the merged output regardless of whether existing scripts use the same names, and works correctly with both the updated server (aspire: prefixed) and stale server (non-prefixed) package versions. Also updates ts-starter template to consistently use aspire: prefixed scripts as canonical names with non-prefixed convenience aliases. Includes 14 unit tests covering conflicts, aliases, dependencies, edge cases, idempotency, and both stale/updated server scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Create src/Shared/NpmVersionHelper.cs with TryParseNpmVersion and ShouldUpgrade methods, file-linked into both Aspire.Cli and Aspire.Hosting.CodeGeneration.TypeScript - Update PackageJsonMerger.MergeDependencySection to upgrade existing deps when scaffold version is strictly newer (semver comparison) - Unparseable ranges (||, workspace:*, file:, link:) are preserved - Refactor TypeScriptLanguageSupport to use shared helper, removing ~50 lines of duplicated private methods - Add 6 new unit tests for semver-aware merging scenarios - Update 2 existing tests to reflect new upgrade-when-newer behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add type guards (JsonValue + TryGetValue) before GetValue<string>() in MergeScripts and MergeDependencySection to prevent InvalidOperationException on non-string JSON values - Bump merge failure log level from Debug to Warning for visibility - Extract PrepareLocalChannel helper from TypeScriptPolyglotTests and TypeScriptStarterTemplateTests into shared CliE2ETestHelpers - Add 8 new robustness tests: non-string scripts, non-string deps, array dep section, JSON root as array, *, latest, pre-release version comparison Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add MergeEngines() that overwrites engines.node with the scaffold's required Node version constraint (needed for ESLint 10 compatibility). Other engines sub-keys (e.g., npm) are preserved from existing. - Add guard in MergeObjects that throws InvalidOperationException if the scaffold template contains an array property — ensures developers add explicit merge logic rather than silently dropping data via DeepMerge. - Let InvalidOperationException propagate through the outer catch (it indicates a programming error, not a runtime merge failure). - Use 'as JsonObject' instead of .AsObject() to safely handle non-object JSON roots without throwing. - Add 4 new tests: engines overwrite, other keys preserved, engines added when missing, array property throws. - Update PreservesNonScriptProperties test for new engines behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TypeScriptLanguageSupport used default JsonSerializerOptions which encodes >= as \u003E=. Add UnsafeRelaxedJsonEscaping (same as PackageJsonMerger) so the engines.node value is written as literal >=24 in the generated package.json. Add assertion to verify no unicode escapes appear in the raw scaffold output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s for brownfield, LoadExisting hardening - Revert TypeScriptLanguageSupport from public to internal - Add private:true to greenfield scaffold output - Move engines.node outside greenfield-only block so brownfield also gets it - Harden LoadExistingPackageJson with try-catch for malformed JSON - Fix duplicate <summary> XML doc in TypeScriptStarterTemplateTests - Add brownfield engines.node and private assertions to tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h, log style - Explicitly set 2-space indent (npm standard) with explanatory comment - Make logger non-nullable; tests use NullLogger.Instance - Add JsonCommentHandling.Skip + AllowTrailingCommas for real-world package.json - Split Merge into parse phase and merge phase with separate try/catch blocks - End log messages with periods (style convention) - Add test for package.json with comments and trailing commas Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The PackageJsonMerger threw InvalidOperationException when encountering array properties like 'keywords' in the existing package.json. These arrays get echoed through the server-side scaffold and both sides have them during merge. Instead of throwing, preserve the existing array (existing-wins semantics) and add scaffold-only arrays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ect logging - Add RuntimeSpec.MigrationFiles and auto-create tsconfig.apphost.json on first run for existing 13.2.0 projects (fixes tsconfig breaking change) - Remove LoadExistingPackageJson; scaffold now produces Aspire-only content so CLI-side PackageJsonMerger handles all merging (fixes double-merge ordering dependency) - Thread ILogger through PackageJsonMerger internals; EnsureObject now logs a warning when replacing non-object values (fixes silent data loss) - Add comprehensive tests for all three fixes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…est helper - Make EnsureObject logger parameter non-nullable (consistent with Merge) - Add CreateRuntime helper to GuestRuntimeTests with ITestOutputHelper logging for better debuggability on test failures - Replace all NullLogger.Instance usages with the test logger Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
22013d4 to
cea52a0
Compare
Description
This draft PR makes TypeScript
aspire initsafer for brownfield repos that already have a frontend project at the repo root, such as a Vite app created withnpm create vite.It updates TypeScript AppHost scaffolding to preserve an existing root
package.jsonandtsconfig.jsoninstead of overwriting them, adds Aspire-owned scripts under non-conflicting names (aspire:start,aspire:build,aspire:dev), emitstsconfig.apphost.jsonfor AppHost-specific compilation, and updates the runtime spec to use that dedicated config.It also adds focused validation for the scenario:
TypeScriptLanguageSupportmerge/runtime behaviorFixes #15122
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: