Dedupe shared backchannel types #10058
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR deduplicates the shared backchannel types between the CLI and the Hosting projects to avoid maintenance of duplicate source declarations.
- Merges the backchannel data types into a single shared file with conditional namespace directives.
- Removes duplicate type definitions from the CLI project.
- Updates the CLI project file to compile the shared backchannel file.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Aspire.Hosting/Backchannel/BackchannelDataTypes.cs | Added conditional directives to set the namespace based on build. |
| src/Aspire.Cli/Backchannel/BackchannelDataTypes.cs | Removed duplicate type definitions now shared by the Hosting project. |
| src/Aspire.Cli/Aspire.Cli.csproj | Updated project file to include the shared BackchannelDataTypes.cs. |
DeagleGross
left a comment
DeagleGross
left a comment
There was a problem hiding this comment.
Looks good for me, but probably Eric can confirm if AOT works fully with such a change
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
Will be good to get this in.
captainsafia
force-pushed
the
safia/dedupe-backchannel-types
branch
from
12407f7 to
95ec908
Compare
src/Aspire.Cli/Aspire.Cli.csproj
Outdated
| <PackageId>Aspire.Cli</PackageId> | ||
| <RollForward>Major</RollForward> | ||
| <PackageTags>aspire cli</PackageTags> | ||
| <DefineConstants>$(DefineConstants);CLI</DefineConstants> |
There was a problem hiding this comment.
| <DefineConstants>$(DefineConstants);CLI</DefineConstants> |
This isn't necessary. It is set above.
| /// <summary> | ||
| /// Specifies the type of input for a publishing prompt input. | ||
| /// </summary> | ||
| internal enum InputType |
There was a problem hiding this comment.
Should this stay just in the CLI? I see there is another "public enum InputType" in Aspire.Hosting, which is used on that side. Having 2 in Aspire.Hosting will probably be confusing.
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
Just 2 nit-ish comments. Looks good beyond those!
src/Aspire.Cli/Aspire.Cli.csproj
Outdated
| <Compile Include="$(SharedDir)PathNormalizer.cs" Link="Utils\PathNormalizer.cs" /> | ||
| <Compile Include="$(SharedDir)CircularBuffer.cs" Link="Utils\CircularBuffer.cs" /> | ||
| <Compile Include="$(SharedDir)StringComparers.cs" Link="StringComparers.cs" /> | ||
| <Compile Include="$(RepoRoot)src\Aspire.Hosting\Backchannel\BackchannelDataTypes.cs" Link="Backchannel\BackchannelDataTypes.cs" /> |
There was a problem hiding this comment.
This causes problems in VS because there are 2 files with the same name in the same folder in the Solution Explorer. So only 1 file shows. Maybe rename src/Aspire.Cli/Backchannel/BackchannelDataTypes.cs => src/Aspire.Cli/Backchannel/CliBackchannelDataTypes.cs
Avoid have duplicate source declarations of the same RPC-transmitted types.
cc: @mitchdenny @adamint