Skip to content

Conversation

@bijington
Copy link
Contributor

…allowed

Description of Change

Restricts the types allowed into AddXPopup extension methods to prevent ContentPage from being passed in.

Linked Issues

PR Checklist

Additional information

Copilot AI review requested due to automatic review settings June 13, 2025 17:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

The PR tightens the generic constraints on AddPopup extension methods to only allow View types (preventing ContentPage usage) and updates related XML docs.

  • Changed all TPopupView constraints from IView to View
  • Updated <typeparam> XML comments to reference View instead of IView
  • Ensured AddPopup overloads correctly restrict input types
Comments suppressed due to low confidence (3)

src/CommunityToolkit.Maui/Extensions/ServiceCollectionExtensions.shared.cs:38

  • The XML doc indicates TPopupViewModel is constrained to INotifyPropertyChanged, but the code uses where TPopupViewModel : notnull. Update the constraint or the doc to match.
/// <typeparam name="TPopupViewModel">The type of the ViewModel to add. Constrained to 

src/CommunityToolkit.Maui/Extensions/ServiceCollectionExtensions.shared.cs:52

  • This overload is AddSingletonPopup, but the doc still says ServiceLifetime.Transient. It should reference ServiceLifetime.Singleton.
/// <see cref="IServiceCollection"/> with <see cref="ServiceLifetime.Transient"/> lifetime.

src/CommunityToolkit.Maui/Extensions/ServiceCollectionExtensions.shared.cs:68

  • The documentation for the AddSingletonPopup<TPopupView, TPopupViewModel> overload still mentions Transient lifetime; this should be updated to Singleton.
/// <see cref="IServiceCollection"/> with <see cref="ServiceLifetime.Transient"/> lifetime.

@bijington bijington enabled auto-merge (squash) June 13, 2025 18:00
@TheCodeTraveler TheCodeTraveler added the breaking change This label is used for PRs that include a breaking change label Jun 13, 2025
Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Shaun! I've added the breaking-change label since this technically modifies our API surface.

Let's discuss internally if it merits another major version. It may not impact many (if any) developers.

@bijington
Copy link
Contributor Author

Thanks Shaun! I've added the breaking-change label since this technically modifies our API surface.

Let's discuss internally if it merits another major version. It may not impact many (if any) developers.

Yeah that's fair. Although it is breaking the rules we're too lax and if a dev provided something like ContentPage it broke. I guess there was the possibility of a dev creating their own IView implementation but I suspect that chance is very low

@bijington bijington merged commit 98c6848 into main Jun 13, 2025
10 checks passed
@bijington bijington deleted the feature/sl-restrict-add-popup branch June 13, 2025 19:33
@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking change This label is used for PRs that include a breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants