-
Notifications
You must be signed in to change notification settings - Fork 429
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
Fixed the unbound memory growth of unidirectional bindings. #176
Conversation
This doesn't seem the only leak though... |
Changes look good, but I agree that the naming is unfortunate. I initially wondered why With that said, what are your thoughts on |
Based on the API guidelines, it should be |
It seems there are some edgy cases in the binding model that haven't been caught. Our primitives themselves aren't leaking. |
Can we add a test for this? |
I actually like the name as-is. The reality is that a |
Probably not. There is no public or internal API to catch the number of observers. |
Even if it can't be observed directly, it can be observed. At any rate, this method is clearly side-effecting, so it deserves a direct verb name. Something like
If it's |
True without this fix, where the We may setup some burning tests that compares memory before and after the burning though. |
I am not disagreeing with that. Just that |
would leave the disposable of a disposed binding in the lifetime signal forever (until the lifetime ends).
I also notice one thing —
observe
would show up in the code completion ofProperty
andSignalProducer
, but neither use this term. Perhaps it would be better to rename it tobind(_:during:)
.