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

Adding CancellationBag #345

Closed
wants to merge 4 commits into from
Closed

Conversation

hfossli
Copy link

@hfossli hfossli commented Jan 13, 2021

As per discussion on https://forums.swift.org/t/how-can-the-composable-architecture-really-become-composable/43045/2

This can be instead or in addition to #335

Let me know if you approve of this direction and I'll spend time writing unit tests and documentation. ☀️

@hfossli
Copy link
Author

hfossli commented Jan 13, 2021

This is of course an incremental non breaking change.

@chrisballinger
Copy link

@hfossli Thank you for this! I think your solution is much more ergonomic and developer-friendly than the LifecycleAction approach.

In my case I was hitting this issue (e.g. Action was received by an optional reducer when its state was "nil") for what I think would be a relatively common scenario of:

  • starting up a database observation, based on a database identifier in State during onAppear
  • tearing down the observation during onDisappear

So far using your approach in this PR has fixed the crashes that I was seeing related to that workflow, and I like that the cancelAll approach ensures that any other dangling Cancellables don't get missed.

}
}
}

var cancellationCancellables: [AnyHashable: Set<AnyCancellable>] = [:]
let cancellablesLock = NSRecursiveLock()
fileprivate struct Lock {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TCA already provides an extension on NSRecursiveLock for sync located in Internal/Locking.swift

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, just wanted a simple standalone file that could be copied out and in to any project

@andreyz
Copy link
Contributor

andreyz commented Feb 3, 2021

@hfossli I think it's possible to extend Reducer with a method (that calls optional internally), that will simply cancel all local effects on nilling of the passed in reducer. To me this seems like a straightforward approach that doesn't require introduction of a new concept like "bags".

@hfossli
Copy link
Author

hfossli commented Feb 3, 2021

Interesting! I'm all ears. Do you have some example? I couldn't find any simple solutions.

return subject.handleEvents(
receiveCompletion: { _ in cancellationCancellable.cancel() },
receiveCancel: cancellationCancellable.cancel
)
}
.eraseToEffect()

return cancelInFlight ? .concatenate(.cancel(id: id), effect) : effect
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be:

    return cancelInFlight ? .concatenate(.cancel(id: id, bag: bag), effect) : effect

@stephencelis
Copy link
Member

@hfossli I'm going to close this out in tandem with #335 (comment)

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants