Change the default of propagateCancel argument in CancelableOperation.then
#352
Comments
|
This looks like the only use of I'm guessing that what we're trying to do here is less about determining whether we should release a breaking change, and more about figuring out whether this is actually breaking (no real uses, nobody relying on the current behavior, ...). (we may want to in-line some of the comment from dart-archive/async#211 (comment) into here - the google3 impact, ...) |
|
The original issue is #320, dart-archive/async#81 was a first version which has some possibly relevant discussion, and dart-archive/async#83 is the final pull request. There may have been a discussion internal to Google, but I don't work there anymore so can't check. The use case for not propagating is if you want to chain multiple |
Closes #212 Authors are more likely to expect propagation, and not cancelling is often a hidden inefficiency so it often goes unnoticed and untested. Update the doc comment to focus on the 3 ways an operation can end - as a value, an error, or a cancellation. This will hopefully make it more clear that the `onCancel` callback does not relate to the cancellation of the returned operation.
Closes #212 Authors are more likely to expect propagation, and not cancelling is often a hidden inefficiency which may go unnoticed and untested. Update the doc comment to focus on the 3 ways an operation can end - as a value, an error, or a cancellation. This will hopefully make it more clear that the `onCancel` callback does not relate to the cancellation of the returned operation.
Closes dart-lang/async#212 Authors are more likely to expect propagation, and not cancelling is often a hidden inefficiency which may go unnoticed and untested. Update the doc comment to focus on the 3 ways an operation can end - as a value, an error, or a cancellation. This will hopefully make it more clear that the `onCancel` callback does not relate to the cancellation of the returned operation.
See discussion in dart-archive/async#211 (comment)
I haven't been able to find any tests that fail when I change the default, but it's hard to say for sure if there could be untested behavior.
I'm fairly confident that some of the usage I saw which does not pass the argument would expect cancellation to propagate.
cc @TastyPi - do you recall why you chose not propagating as the default? Do you have any use cases where propagating the cancel would result in incorrect behavior?
cc @lrhn @devoncarew - I'm tempted to try to roll out a change in the default as a minor release. I'm concerned that the unsafe default is going to cause more pain than potentially breaking an existing usage. WDYT?
The text was updated successfully, but these errors were encountered: