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: ensure synchronous command execution is lazy #1274

Merged
merged 2 commits into from Feb 20, 2017

Conversation

Projects
None yet
3 participants
@kentcb
Copy link
Contributor

kentcb commented Feb 15, 2017

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

Bug fix.

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

Synchronous commands execute their workload immediately upon a call to Execute instead of requiring a subscription.

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

Synchronous commands now require a subscription in order for their workload to execute.

The documentation clearly states that subscription is required before commands execute, and that is indeed how asynchronous commands work. So this change brings the code into line with the documentation.

Does this PR introduce a breaking change?

Technically, I suppose I'd have to say yes. However, the circumstances under which this would break someone are:

  1. They are using a synchronous command (which are less common than asynchronous commands, in my experience).
  2. They are executing the command and not subscribing (which is less common than binding the command or using InvokeCommand).

So anyone who does break isn't using the framework correctly and would be bitten if, for example, they tried the same thing with an asynchronous command. Also, if they're subscribed to IsExecuted or to the command itself, those subscriptions will never fire if they are not subscribing.

Please check if the PR fulfills these requirements

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 15, 2017

Coverage Status

Coverage increased (+0.2%) to 66.133% when pulling 111ad1f on kentcb:lazy-sync-commands into 8e9963f on reactiveui:develop.

@Qonstrukt

This comment has been minimized.

Copy link
Member

Qonstrukt commented Feb 15, 2017

It's quite inconsistent alright, but to be fair, I always thought it was a design decision. Since the code in the command isn't asynchronous, there's not much reason to "subscribe" onto it. But that's an assumption on my part!

The reason I'm telling is because we currently do have (some) code that functions without the Subscribe part because we thought it was by design. And hastily upgrading RxUI would render those functions un-executed, maybe even without noticing. Since unit-testing platform-code is still a hassle it might very well go undetected. (Since the caller is often the View in the platform-code.)

I'm not saying this is a bad change, I'm all for consistency, but we need to make sure to inform users well of this. 👍

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage increased (+0.2%) to 66.688% when pulling eaac01f on kentcb:lazy-sync-commands into 427885e on reactiveui:develop.

@kentcb kentcb changed the title Ensure synchronous command execution is lazy fix: ensure synchronous command execution is lazy Feb 20, 2017

@kentcb kentcb added this to the vNext milestone Feb 20, 2017

@kentcb kentcb merged commit d9d36e5 into reactiveui:develop Feb 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.2%) to 66.688%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment