[Automated] Update API Surface Area #12058
Conversation
github-actions
bot
added
the
NO-MERGE
github-actions
bot
requested review from
ReubenBond and
eerhardt
as code owners
dotnet-policy-service
bot
added
Re-opened Github-Action PR
and removed
Closed Github-Action PR
labels
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12058Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12058" |
github-actions
bot
force-pushed
the
update-api-diffs
branch
8 times, most recently
from
11736e0 to
606eb89
Compare
github-actions
bot
force-pushed
the
update-api-diffs
branch
7 times, most recently
from
34595b8 to
00cfbc8
Compare
github-actions
bot
force-pushed
the
update-api-diffs
branch
3 times, most recently
from
ed206b6 to
ea7c329
Compare
src/Aspire.Hosting.Azure.AppService/api/Aspire.Hosting.Azure.AppService.cs
Show resolved
Hide resolved
| //------------------------------------------------------------------------------ | ||
| namespace Aspire.Hosting | ||
| { | ||
| public partial class JavaScriptAppResource : ApplicationModel.ExecutableResource, IResourceWithServiceDiscovery, ApplicationModel.IResourceWithEndpoints, ApplicationModel.IResource, IResourceWithContainerFiles |
There was a problem hiding this comment.
This should move to namespace Aspire.Hosting.NodeJs
There was a problem hiding this comment.
Maybe we should just completely revamp this library and making its name and namespace Aspire.Hosting.JavaScript.
There was a problem hiding this comment.
#12681 renames NodeJs to JavaScript
| public bool TryAdd(NetworkIdentifier networkID, ValueSnapshot<AllocatedEndpoint> snapshot) { throw null; } | ||
| } | ||
|
|
||
| public readonly partial struct NetworkIdentifier : System.IEquatable<NetworkIdentifier> |
There was a problem hiding this comment.
Does this need to be a record struct?
There was a problem hiding this comment.
Honestly, I am not familiar with .NET API guidelines regarding record struct usage, but I thought it would make sense here because I expect this might eventually evolve into a compound identifier where equality is defined as "all properties are equal".
There was a problem hiding this comment.
Hmm, if we want to add members to it in the future, that will be a binary breaking change according to
https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules#members
❌ DISALLOWED: Adding an instance field to a struct that has no nonpublic fields
If a struct has only public fields or has no fields at all, callers can declare locals of that struct type without calling the struct's constructor or first initializing the local to default(T), so long as all public fields are set on the struct before first use. Adding any new fields - public or nonpublic - to such a struct is a source breaking change for these callers, as the compiler will now require the additional fields to be initialized.
Additionally, adding any new fields - public or nonpublic - to a struct with no fields or only public fields is a binary breaking change to callers that have applied [SkipLocalsInit] to their code. Since the compiler wasn't aware of these fields at compile time, it could emit IL that doesn't fully initialize the struct, leading to the struct being created from uninitialized stack data.
If a struct has any nonpublic fields, the compiler already enforces initialization via the constructor or default(T), and adding new instance fields is not a breaking change.
There was a problem hiding this comment.
OK so make it a class with public fields? (just Value or Id for starters)?
There was a problem hiding this comment.
Do we know we want to eventually evolve it to a compound identitier? If so, yeah maybe making it a class or record would be better.
There was a problem hiding this comment.
I am not certain about it but it would be really unfortunate if we closed this possibility now. I am leaning towards record
github-actions
bot
force-pushed
the
update-api-diffs
branch
from
d7651c5 to
62f6d43
Compare
This makes the library consistent to use language naming, and not a single technology. It allows for more JS features in the future that aren't tied to Node.js. Since the library is being renamed, I also removed the Obsolete APIs since there is no reason to ship them in the new library. Contributes to dotnet#12058
* API Review feedback See comments in #12058 * Change InputsDialogValidationContext ServiceProvider * update suppression file * Remove ResourceAnnotationMutationBehavior from WithContainerFilesSource. Replace should use ClearContainerFilesSources now. * Move JavaScriptAppResource to Aspire.Hosting.NodeJs namespace * Make ExecutableLaunchConfiguration internal * Make DeploymentStateManagerBase and subclasses internal * make const and static fields on ContainerCertificatePathsAnnotation internal. * Fix DcpVisibilityTests * Move EndpointHostHelpers to the Aspire.Hosting.ApplicationModel namespace. * Move IDeploymentStateManager and DeploymentStateSection from Publishing to Pipelines. Make DeploymentStateSection experimental. * Make strings consistently const. * Make ManifestPublishingExtensions internal. * Rename PipelineStepExtensions to PipelineStepFactoryExtensions Rename PipelineStepsExtensions to PipelineStepExtensions
See comments in #12058
This makes the library consistent to use language naming, and not a single technology. It allows for more JS features in the future that aren't tied to Node.js. Since the library is being renamed, I also removed the Obsolete APIs since there is no reason to ship them in the new library. Contributes to dotnet#12058
* Rename Aspire.Hosting.NodeJs to Aspire.Hosting.JavaScript This makes the library consistent to use language naming, and not a single technology. It allows for more JS features in the future that aren't tied to Node.js. Since the library is being renamed, I also removed the Obsolete APIs since there is no reason to ship them in the new library. Contributes to #12058 * Fix build * Fix Helix tests
* Rename Aspire.Hosting.NodeJs to Aspire.Hosting.JavaScript This makes the library consistent to use language naming, and not a single technology. It allows for more JS features in the future that aren't tied to Node.js. Since the library is being renamed, I also removed the Obsolete APIs since there is no reason to ship them in the new library. Contributes to dotnet#12058 * Fix build * Fix Helix tests
…12687) * Rename Aspire.Hosting.NodeJs to Aspire.Hosting.JavaScript This makes the library consistent to use language naming, and not a single technology. It allows for more JS features in the future that aren't tied to Node.js. Since the library is being renamed, I also removed the Obsolete APIs since there is no reason to ship them in the new library. Contributes to #12058 * Fix build * Fix Helix tests
Auto-generated update to the API surface to compare current surface vs latest release. This should only be merged once this surface area ships in a new release.