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

Fix to handle Cancellation Token Tasks for ReactiveCommand.CreateFromTask #3704

Merged
merged 5 commits into from
Jan 1, 2024

Conversation

ChrisPulman
Copy link
Member

What kind of change does this PR introduce?

Fix for #1245
Fix for #2153
Fix for #3450

What is the current behavior?

ReactiveCommand does not properly support Cancellation tokens properly for CreateFromTask due to an underlying issue in System.Reactive

What is the new behavior?

Fix the issues with the base functions within ReactiveCommand due to an issue with Observable.FromAsync from System.Reactive by using a new ObservableMixins.FromAsyncWithAllNotifications as the new function, this extends Observable.FromAsync handling the error bubbling as required.

ObservableMixins.FromAsyncWithAllNotifications can be used to transform a Cancellation Task into an Observable producing the expected cancellation, errors and results.

What might this PR break?

ReactiveCommand.CreateFromTask will now handle exceptions as expected, any existing workarounds could be removed once tested with actual implementation in end users code.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

Co-authored-by: @idg10 - created the base code in #3556

Copy link

codecov bot commented Dec 31, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (2d7801e) 58.39% compared to head (a5d344c) 58.88%.

Files Patch % Lines
src/ReactiveUI/Mixins/ObservableMixins.cs 49.01% 26 Missing ⚠️
src/ReactiveUI/ReactiveCommand/ReactiveCommand.cs 82.60% 11 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3704      +/-   ##
==========================================
+ Coverage   58.39%   58.88%   +0.48%     
==========================================
  Files         160      160              
  Lines        5776     5842      +66     
  Branches     1029     1031       +2     
==========================================
+ Hits         3373     3440      +67     
+ Misses       2014     2012       -2     
- Partials      389      390       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisPulman ChrisPulman merged commit fb800f0 into main Jan 1, 2024
3 checks passed
@ChrisPulman ChrisPulman deleted the CP_FixForReactiveCommandCancelation branch January 1, 2024 00:16
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants