Closed sisoje closed 3 years ago
You're right, calling cancel()
is not needed, but there is just one line in the project where it appears.
Other than that, I would argue that removing CancelBag
is worth it. CancelBag
is just as convenient as Set<AnyCancellable>
, but has a clearer purpose and strictly scoped API. More importantly, CancelBag
is a reference type, as opposed to Set
, which allows for more predictable memory management in certain places, like Loadable
enum.
I'm a big fan of cleaning things up, and you're welcomed to post a code snippet for comparison, but I really cannot see how Set<AnyCancellable>
would make the code more concise or clearer.
This project is overengineered enough (in a good way) I just think removing CancelBag would make it clearer. While you mention Loadable, I like it and used similar stuff in my project but I just made my isLoading case like:
case loading(AnyCancellable)
Would you like me to prepare a PR with removed cancel bag?
PS I love swift for not being garbage-collected language, and I'd say that there is no such thing as "more predictable memory management" in swift. Its the same whether you use value or reference.
I cannot forbid you from doing this, of course! Before you do, have a look at this additional feature of CancelBag
in the mvvm branch.
This trick actually has a practical application.
I even have a post on my blog about it :)
memory management is the same whether you use value or reference.
No.
ok i see this additional feature of cancelbag... well ok for the fans "sugar sythax" this function builder may be also a good think to learn 👍
AnyCancellable is doing cancel on deinit, it called RAII pattern: https://developer.apple.com/documentation/combine/anycancellable
No need to call explicit cancel() on items. CancelBag looks like its not needed. It will cleanup a lot of code