Http-client-js server alloy rewrite#10372
Http-client-js server alloy rewrite#10372timotheeguerin wants to merge 15 commits intomicrosoft:mainfrom
Conversation
|
You can try these changes here
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR rewrites the @typespec/http-server-csharp emitter to use Alloy/JSX components and the TypeSpec emitter-framework, alongside updates to build/test configuration and supporting utilities.
Changes:
- Replaces the legacy asset-emitter–based implementation with a new Alloy-based emitter entrypoint and JSX component set.
- Updates TypeScript/Vitest/build configuration to support TSX, Alloy build, and colocated
src/**/*.test.*tests. - Adds new utilities (naming, scalar/type mapping, HTTP helpers, port selection) with unit tests, while removing large legacy implementation files.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.ws.json | Removes the http-server-csharp project reference from the workspace TS build graph. |
| packages/http-server-csharp/vitest.config.ts | Updates Vitest discovery/setup and adds Alloy Rollup plugin + resolve conditions. |
| packages/http-server-csharp/tsconfig.json | Switches to NodeNext + TSX settings and declaration-only emit. |
| packages/http-server-csharp/tsconfig.build.json | Narrows build include/exclude for TS/TSX and omits tests/testing helpers. |
| packages/http-server-csharp/test/test-host.ts | Adjusts import path for CSharpServiceEmitterOptions. |
| packages/http-server-csharp/src/utils/type-mapping.ts | Adds intrinsic-scalar→C# mapping and collection type resolution helpers. |
| packages/http-server-csharp/src/utils/type-mapping.test.ts | Adds unit tests for scalar mapping, collection type, and well-known types. |
| packages/http-server-csharp/src/utils/port.ts | Adds helper to pick a free TCP port by probing. |
| packages/http-server-csharp/src/utils/naming.ts | Adds C# identifier casing/escaping and invalid-identifier transformation helpers. |
| packages/http-server-csharp/src/utils/naming.test.ts | Adds unit tests for naming and identifier validation helpers. |
| packages/http-server-csharp/src/utils/http-helpers.ts | Adds HTTP verb/status/parameter binding helpers for ASP.NET generation. |
| packages/http-server-csharp/src/utils/http-helpers.test.ts | Adds unit tests for HTTP helper functions. |
| packages/http-server-csharp/src/testing/vitest.setup.ts | Adds Alloy testing setup import for Vitest. |
| packages/http-server-csharp/src/testing/tester.ts | Adds shared TypeSpec Tester instance factory for emitter tests. |
| packages/http-server-csharp/src/testing/index.ts | Exposes $lib under a ./testing entrypoint. |
| packages/http-server-csharp/src/lib/type-helpers.ts | Removes legacy type helpers (now replaced by new utilities/components). |
| packages/http-server-csharp/src/lib/service.ts | Removes the legacy asset-emitter–based server emitter implementation. |
| packages/http-server-csharp/src/lib/scaffolding.ts | Removes legacy scaffolding generator implementation. |
| packages/http-server-csharp/src/lib/project.ts | Removes legacy project file generator implementation. |
| packages/http-server-csharp/src/lib/lib.ts | Updates option docs and removes CSharpServiceOptions singleton. |
| packages/http-server-csharp/src/lib/interfaces.ts | Removes legacy C# type/attribute/asset-emitter interface layer. |
| packages/http-server-csharp/src/lib/index.ts | Redirects $onEmit export to the new ./emitter.js. |
| packages/http-server-csharp/src/lib/doc.ts | Removes legacy docs generator implementation. |
| packages/http-server-csharp/src/lib/boilerplate.ts | Removes legacy C# boilerplate + converter source file generator. |
| packages/http-server-csharp/src/lib/attributes.ts | Removes legacy attribute computation implementation. |
| packages/http-server-csharp/src/emitter.tsx | Adds new Alloy/TypeSpec emitter-framework $onEmit entrypoint. |
| packages/http-server-csharp/src/components/type-expression.tsx | Adds a C# server-specific TypeSpec type→C# type expression renderer. |
| packages/http-server-csharp/src/components/type-expression.test.tsx | Adds tests for TypeExpression scalar/array/record/nullable/literal mappings. |
| packages/http-server-csharp/src/components/serialization/json-converters.tsx | Adds Alloy components emitting static C# JSON helper files. |
| packages/http-server-csharp/src/components/serialization/json-converters.test.tsx | Adds render tests ensuring helper files are emitted. |
| packages/http-server-csharp/src/components/scaffolding/mock-scaffolding.tsx | Adds Alloy components emitting mock scaffolding C# files. |
| packages/http-server-csharp/src/components/scaffolding/mock-scaffolding.test.tsx | Adds render tests for mock scaffolding files. |
| packages/http-server-csharp/src/components/project/program.tsx | Adds Alloy component emitting ASP.NET Program.cs. |
| packages/http-server-csharp/src/components/project/program.test.tsx | Adds render tests for Program.cs variations. |
| packages/http-server-csharp/src/components/project/launch-settings.tsx | Adds Alloy components emitting appsettings + launchSettings.json. |
| packages/http-server-csharp/src/components/project/launch-settings.test.tsx | Adds render tests for launch settings + appsettings. |
| packages/http-server-csharp/src/components/project/csproj.tsx | Adds Alloy component emitting a .csproj file. |
| packages/http-server-csharp/src/components/project/csproj.test.tsx | Adds render tests for .csproj (with/without Swagger package). |
| packages/http-server-csharp/src/components/output.tsx | Adds small wrapper around emitter-framework Output. |
| packages/http-server-csharp/src/components/models.tsx | Adds model enumeration + per-model file emission scaffolding. |
| packages/http-server-csharp/src/components/models.test.tsx | Adds tests using emitter-framework ClassDeclaration for models. |
| packages/http-server-csharp/src/components/interfaces.tsx | Adds business-logic interface generation component. |
| packages/http-server-csharp/src/components/interfaces.test.tsx | Adds tests for business-logic interface generation. |
| packages/http-server-csharp/src/components/enums.tsx | Adds enum enumeration + per-enum file emission scaffolding. |
| packages/http-server-csharp/src/components/enums.test.tsx | Adds tests using emitter-framework EnumDeclaration. |
| packages/http-server-csharp/src/components/controllers.tsx | Adds ASP.NET controller class generation component. |
| packages/http-server-csharp/src/components/controllers.test.tsx | Adds tests for controller class generation. |
| packages/http-server-csharp/src/components/controller-action.tsx | Adds per-operation controller action generation component. |
| packages/http-server-csharp/src/components/controller-action.test.tsx | Adds tests for controller actions (GET, DELETE with path param). |
| packages/http-server-csharp/src/cli/cli.ts | Updates CLI to use new port utility path. |
| packages/http-server-csharp/package.json | Switches build to alloy build, updates exports/main, adds Alloy deps. |
| AGENTS.md | Adds repo guidance pointing to installed Alloy docs. |
| .github/agents/alloy-style.agent.md | Adds an Alloy style-review agent configuration. |
| .github/agents/alloy-language-package-reviewer.agent.md | Adds an Alloy language package reviewer agent configuration. |
| .github/agents/alloy-debugger.agent.md | Adds an Alloy debugging agent configuration and guidance. |
| .github/agents/alloy-architect.agent.md | Adds an Alloy architecture agent configuration and guidance. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (8)
packages/http-server-csharp/src/emitter.tsx:1
- The new
$onEmitcurrently writes only empty directories and does not invoke any of the new components (Models/Enums/Controllers/serialization/scaffolding/project files). This is a functional regression versus the previous emitter, because compiling with@typespec/http-server-csharpwould produce no artifacts. Wire the new component tree into this output (e.g., renderModels,Enums, controller/interface generation, and any required helper files) so the emitter actually generates source files.
packages/http-server-csharp/test/test-host.ts:1 - This import path looks inconsistent with the new folder layout shown in this diff (
src/lib/index.ts,src/lib/lib.ts,src/emitter.tsx). Unless a newsrc/lib.tsbarrel exists elsewhere,../src/lib.jswon't resolve and will break test utilities at runtime. Update the import to the correct module that exportsCSharpServiceEmitterOptions(or add a barrel file that actually compiles todist/src/lib.js).
packages/http-server-csharp/src/testing/index.ts:1 - This re-export targets
../lib.js, but the diff shows the library code living undersrc/lib/(e.g.,src/lib/index.ts,src/lib/lib.ts). Ifsrc/lib.ts/src/lib.jsdoesn't exist, the./testingexport will be broken. Point this to the actual compiled library entry (e.g., thesrc/lib/index.tsoutput) or introduce a barrel module that matches this export.
packages/http-server-csharp/vitest.config.ts:1 - Using only the
developmentcondition for Vitest module resolution is likely incorrect for Alloy-based packages and can lead to duplicate framework instances or the wrong entrypoints being loaded during tests. Consider aligning Vitest's resolve conditions with the package's intended export conditions (commonly including asourcecondition for tests/dev) and ensure SSR resolve conditions are also configured if needed.
packages/http-server-csharp/src/utils/port.ts:1 - Port selection has two concrete issues: (1)
Math.random() * diffnever selectsmax(range is effectively[min, max)), and (2) whentriesis exhausted it returnsminwithout verifying it's free, which can yield an in-use port and cause flaky failures. Make the range inclusive (or document exclusivity) and, on exhaustion, throw or deterministically scan the range and only return a verified free port.
packages/http-server-csharp/src/utils/http-helpers.ts:1 - The comment claims this converts TypeSpec path params, but the implementation is currently a pure passthrough. Either implement the documented conversion (or validation/normalization) or update the comment to match the current behavior to avoid misleading future changes.
tsconfig.ws.json:1 - Removing
packages/http-server-csharp/tsconfig.build.jsonfrom the workspace references can cause this package to be skipped by root TS build workflows (e.g.,tsc -b tsconfig.ws.json) and may lead to CI/build drift if other packages still depend on it. If the intent is to fully migrate away from TS project references, consider updating the root build pipeline accordingly; otherwise, re-add the reference.
packages/http-server-csharp/src/utils/type-mapping.test.ts:1 - A new intrinsic scalar variant (
unixTimestamp32) is added toscalarMapintype-mapping.ts, but the test suite here doesn’t assert its mapping. Add a focused test case forscalarMap.get(\"unixTimestamp32\")to prevent regressions in this server-specific scalar behavior.
| public class HttpServiceExceptionFilter : IExceptionFilter | ||
| { | ||
| public void OnException(ExceptionContext context) | ||
| { | ||
| context.Result = new ObjectResult(new { error = context.Exception.Message }) | ||
| { | ||
| StatusCode = 500 | ||
| }; | ||
| context.ExceptionHandled = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This filter returns context.Exception.Message to clients for all unhandled exceptions, which can leak internal implementation details (and differs from the previous approach that handled a dedicated service exception type). Consider returning a generic error payload by default, and only surfacing controlled details for known/intentional exceptions (e.g., mapping a custom HttpServiceException to a status code and safe response body).
| function getUnixEpochDateTimeConverter(): string { | ||
| return `// Generated by @typespec/http-server-csharp | ||
| // <auto-generated /> | ||
| #nullable enable | ||
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| namespace TypeSpec.Helpers.JsonConverters | ||
| { | ||
| public class UnixEpochDateTimeOffsetConverter : JsonConverter<DateTimeOffset> | ||
| { |
There was a problem hiding this comment.
The function/file naming is inconsistent: getUnixEpochDateTimeConverter() emits a UnixEpochDateTimeOffsetConverter class (and the SourceFile path is UnixEpochDateTimeConverter.cs). Align function name and file name with the emitted class name (or rename the class) to reduce confusion and avoid mismatches in downstream references.
| import { For, SourceDirectory, type Children } from "@alloy-js/core"; | ||
| import * as cs from "@alloy-js/csharp"; | ||
| import { isVoidType, type Model, type ModelProperty } from "@typespec/compiler"; | ||
| import { useTsp } from "@typespec/emitter-framework"; | ||
| import { ClassDeclaration, Property } from "@typespec/emitter-framework/csharp"; | ||
| import { TypeExpression } from "./type-expression.jsx"; |
There was a problem hiding this comment.
models.tsx currently has multiple unused imports in the shown file contents (e.g., SourceDirectory, isVoidType, ModelProperty, Property, TypeExpression). This increases noise and may fail lint/typecheck depending on repo settings. Remove unused imports or start using them as intended.
| import { For, SourceDirectory, type Children } from "@alloy-js/core"; | |
| import * as cs from "@alloy-js/csharp"; | |
| import { isVoidType, type Model, type ModelProperty } from "@typespec/compiler"; | |
| import { useTsp } from "@typespec/emitter-framework"; | |
| import { ClassDeclaration, Property } from "@typespec/emitter-framework/csharp"; | |
| import { TypeExpression } from "./type-expression.jsx"; | |
| import { For, type Children } from "@alloy-js/core"; | |
| import * as cs from "@alloy-js/csharp"; | |
| import { type Model } from "@typespec/compiler"; | |
| import { useTsp } from "@typespec/emitter-framework"; | |
| import { ClassDeclaration } from "@typespec/emitter-framework/csharp"; |
commit: |
|
❌ There is undocummented changes. Run The following packages have changes but are not documented.
Show changes |
|
❌ There is undocummented changes. Run The following packages have changes but are not documented.
Show changes |
No description provided.