-
Notifications
You must be signed in to change notification settings - Fork 133
Remove deprecated IDistributedApplicationLifecycleHook interface #915
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
Remove deprecated IDistributedApplicationLifecycleHook interface #915
Conversation
Co-authored-by: aaronpowell <[email protected]>
Co-authored-by: aaronpowell <[email protected]>
|
You want to replace these with |
don't we want to replace them with the |
|
|
|
@copilot according to @davidfowl we should be using |
Co-authored-by: aaronpowell <[email protected]>
Updated Dapr to use Commit: 69ebc86 |
|
@davidfowl how's that looking? |
| // Only install packages during development, not in publish mode | ||
| if (!resource.ApplicationBuilder.ExecutionContext.IsPublishMode) | ||
| { | ||
| var installerName = $"{resource.Resource.Name}-bun-install"; |
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.
This call isn't idempotent. There's a new helper here https://github.com/dotnet/aspire/blob/5a87acfdb041ce3f89189a8af379fa24df4f3d44/src/Aspire.Hosting/DistributedApplicationBuilderExtensions.cs#L91.
You can use it like so https://github.com/dotnet/aspire/blob/5a87acfdb041ce3f89189a8af379fa24df4f3d44/src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs#L822-L829
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.
But this is moving away from the event model for doing this to the same model used for npm/pnpm/etc. installing.
| if (!resource.ApplicationBuilder.ExecutionContext.IsPublishMode) | ||
| { | ||
| var installerName = $"{resource.Resource.Name}-deno-install"; | ||
| var installer = new DenoInstallerResource(installerName, resource.Resource.WorkingDirectory); |
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.
Same comment as above.
|
Yep! |
Overview
This PR removes all usages of the deprecated
IDistributedApplicationLifecycleHookinterface and replaces them with modern event-based patterns, as required by dotnet/aspire#11266.Changes
Deno & Bun: Installer Resource Pattern
Previously, Deno and Bun used lifecycle hooks to install packages before application startup. This PR migrates them to use the installer resource pattern (consistent with NodeJS npm/yarn/pnpm):
Before:
After:
Files Changed:
DenoInstallerResource.cs,BunInstallerResource.csDenoPackageInstallerLifecycleHook.cs,DenoPackageInstaller.cs,BunPackageInstallerLifecycleHook.csDenoAppHostingExtensions.cs,BunAppExtensions.csDapr: IDistributedApplicationEventingSubscriber Pattern
Dapr's lifecycle hook has been converted to use the
IDistributedApplicationEventingSubscriberpattern for idempotent registration:Before:
After:
Files Changed:
IDistributedApplicationBuilderExtensions.cs,DaprDistributedApplicationLifecycleHook.csPattern Benefits:
TryAddEventingSubscriber(similar to oldTryAddLifecycleHook)Tests
Updated tests to use standard public APIs instead of internal lifecycle hook methods:
ExecuteBeforeStartHooksAsync()withapp.StartAsync()UnsafeAccessorreflection codeBenefits
IDistributedApplicationLifecycleHookIDistributedApplicationEventingSubscriberfor proper lifecycle managementMigration Notes
No breaking changes for users:
WithDenoPackageInstallation()andWithBunPackageInstallation()work exactly as beforeAddDapr()continues to work with the same APIFixes #890
Original prompt
Fixes #890
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.