Skip to content
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

Add CancellationToken Parameter to Methods Returning Task and ValueTask #1503

Merged
merged 55 commits into from Nov 10, 2023

Conversation

brminnick
Copy link
Collaborator

@brminnick brminnick commented Nov 7, 2023

Description of Change

This PR updates every method that returns a Task or ValueTask to include a CancellationToken as a parameter.

This ensures that the .NET MAUI Community Toolkit follows Microsoft's recommended async/await Best Practices to always support Task Cancellation.

Requiring a CancellationToken also ensures that analyzer CA2016 ("Forward the 'cancellationToken' parameter methods that take one") correctly alerts developers using our library to propagate their CancellationToken to our APIs.

I've also removed the default values for CancellationToken parameters (eg CancellationToken token = default -> CancellationToken token) . This follows the best-practice recommendation for Libraries to require the developer to make a thoughtful choice as to how to handle cancellation.

Update: Looking at the .NET BCL, their APIs provide default values for CancellationToken. Therefore, we too will provide default values for our CancellationToken parameters (eg Cancellation token = default)).

Example: IDrawingView.GetImageStream

For example, the IDrawingView.GetImageStream method changes from

ValueTask<Stream> GetImageStream(double imageSizeWidth, double imageSizeHeight, Paint background);

becomes

ValueTask<Stream> GetImageStream(double imageSizeWidth, double imageSizeHeight, Paint background, CancellationToken token = default);

Linked Issues

PR Checklist

Additional information

In the .editorconfig, I've promoted CA1068 to error to ensure that we follow best-practices when adding a CancellationToken as a parameter to methods.

I've added the breaking change This label is used for PRs that include a breaking change label because this modifies existing APIs. I recommend merging this before we publish CommunityToolkit v7.0.0 which contains the existing breaking change targeting .NET 8.

This PR also adds a Timeout to every Unit Test that returns a Task to ensure our Unit Test timeout when a deadlock is encountered.

I've also added do not merge Do not merge this PR label until Unit Tests are added and Documentation is updated.

@brminnick brminnick added pending documentation This feature requires documentation do not merge Do not merge this PR breaking change This label is used for PRs that include a breaking change labels Nov 7, 2023
Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

This is a great idea! I've two comments around the current implementation

@brminnick brminnick removed the pending documentation This feature requires documentation label Nov 8, 2023
@brminnick brminnick removed the do not merge Do not merge this PR label Nov 9, 2023
@brminnick
Copy link
Collaborator Author

brminnick commented Nov 9, 2023

@pictos Ok! This PR is ready for Review (again!).

I've removed the do not merge Do not merge this PR label

@brminnick brminnick merged commit 1fce082 into main Nov 10, 2023
8 checks passed
@brminnick brminnick deleted the Add-CancellationToken-to-Async-Methods branch November 10, 2023 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

3 participants