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

refactor: Remove ReactiveCommand abstract class. #1836

Merged
merged 4 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@glennawatson
Copy link
Contributor

glennawatson commented Nov 7, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This removes the abstract base class we use in ReactiveCommand.

What is the current behavior? (You can also link to an open issue here)
There is a abstract base class. Users can introduce subtle bugs by using it directly in their code. It was meant for interop with the ICommand but now ReactiveCommandBase<TParam, TResult> now implements that logic. There is also a interface allow some of the Command Binding code to keep functioning and exposes the observables for the user. It's deliberately not derived from ICommand due to us wanting the separation.

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

What might this PR break?
Users who have a derived off the abstract base class. They will have to change their classes to derive from ReactiveCommandBase<TParam, TResult> now.

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:
Docs have already been modified to show the non-abstract class use.

This relates to reactiveui/rfcs#19

@glennawatson glennawatson requested review from reactiveui/core-team as code owners Nov 7, 2018

Made the IReactiveCommand internal again. Because it doesn't derive o…
…ff ICommand it can be an issue for binding etc
@RLittlesII
Copy link
Contributor

RLittlesII left a comment

Glenn. This LGTM. I would say we need another member of the @reactiveui/core-team to make sure there are no major concerns.

@olevett
Copy link
Member

olevett left a comment

Looks good, assume this is a reasonably major version change?

@glennawatson

This comment has been minimized.

Copy link
Contributor

glennawatson commented Nov 7, 2018

Will take us to 9.1 or 10. If we follow semantic versioning I guess 10

@worldbeater
Copy link
Member

worldbeater left a comment

LGTM!

@glennawatson glennawatson dismissed stale reviews from worldbeater, olevett, and RLittlesII via d691915 Nov 7, 2018

@glennawatson glennawatson requested a review from reactiveui/maintainers as a code owner Nov 7, 2018

@glennawatson glennawatson merged commit 502963d into master Nov 8, 2018

1 check passed

license/cla All CLA requirements met.
Details

@delete-merged-branch delete-merged-branch bot deleted the glennawatson-remove-reactivecommand-abstract branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment