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: several ReactiveCommand bugs #1534

Merged
merged 1 commit into from
Nov 12, 2017
Merged

Conversation

kentcb
Copy link
Contributor

@kentcb kentcb commented Nov 10, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

fixes

What is the current behavior? (You can also link to an open issue here)

  1. InvokeCommand relies on Rx's WithLatestFrom operator, which is...weird. It subscribes to first observable first, and then the second, which can result in missing values.
  2. Multiple in-flight command executions are not supported. More specifically, a count of in-flight executions is not kept, so it behaves a little weird (saying it can execute when another execution may be in flight).
  3. Disposing of an in-flight execution subscription is now handled correctly. Fixes Disposing of an observable involving a ReactiveCommand will leave the ReactiveCommand with CanExecute set to false #1525.

What is the new behavior (if this is a feature change)?

  1. InvokeCommand relies on our own custom WithLatestFromFixed operator, which publishes the source observable so values aren't missed.
  2. A count of executions is tracked so that IsExecuting and CanExecute report intuitive values.
  3. Termination of execution observables is correctly handled, such that disposing an in-flight execution does not screw things up.

What might this PR break?

Nothing - it should fix several scenarios.

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:

@dnfclas
Copy link

dnfclas commented Nov 10, 2017

@kentcb,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@Qonstrukt
Copy link
Member

Qonstrukt commented Nov 10, 2017

I've merged this locally and can confirm this fixes #1476. 👍

@kentcb
Copy link
Contributor Author

kentcb commented Nov 10, 2017

I meant to link to the related Rx issue.

@kentcb
Copy link
Contributor Author

kentcb commented Nov 10, 2017

Thanks for confirming @Qonstrukt! I've rebased the branch on top of your Xamarin.Android fixes and will hopefully get it merged a bit later today. Reviews welcome!

@kentcb kentcb force-pushed the invoke-command branch 2 times, most recently from f6d7ab8 to da42ed1 Compare November 11, 2017 00:56
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling abb841d on kentcb:invoke-command into ** on reactiveui:develop**.

@kentcb kentcb changed the title fix: use custom WithLatestFrom instead of buggy Rx one fix: several ReactiveCommand bugs Nov 11, 2017
@kentcb kentcb added this to the vNext milestone Nov 12, 2017
@kentcb kentcb merged commit 05fed1a into reactiveui:develop Nov 12, 2017
@kentcb
Copy link
Contributor Author

kentcb commented Nov 12, 2017

Thanks for the review @olevett

glennawatson pushed a commit that referenced this pull request Mar 23, 2019
 
fix: several ReactiveCommand bugs (#1534)

* Use custom WithLatestFrom implementation that bypasses unexpected behavior in the operator provided by Rx, thereby fixing InvokeCommand scenarios in which the source is cold or primed.
* Support multiple, in-flight executions correctly.
* Correctly handle disposal of an in-flight execution.
@lock lock bot locked and limited conversation to collaborators Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants