Stop fatal error on non convertible value in 'RKKeyValueSignal'

[AnthonyMDev]

I don't believe it is proper behavior here for Bond to cause my application to crash when an RKKeyValueSignal gets a change that is not convertible to the signal's type. I would instead like to see the signal send me an error in this case. The user should be able to handle this error as they see fit, rather than having an application crash.

I have found this causing many crashes in my application when using NSManagedObjects and deallocating multiple view controllers, (i.e. when popping to the root view controller of a navigation stack). This is because I am deallocating the managed object's context simultaneously to the deallocation of the view controllers. The context is set to nil for the objects and a KVO event is fired with a value of nil. The signals, which will be shortly disposed of, but have not been yet, cannot convert the nil value to the expected type and cause a fatal error.

Thoughts?

[srdanrasic]

You are right, we should improve that. Let me tackle the problem alongside the update I'm doing for #288.

[AnthonyMDev]

Thanks! You know I'm always willing to make PRs but I'm not sure exactly how you want to tackle this one, so if you are already handling it that's great.

As always, thanks for all your hard work.

[AnthonyMDev]

@srdanrasic I'd really love to see this implemented soon. I know you're probably super busy, but if you have the time to even discuss what for you are thinking these errors would take, I'd be happy to help with a PR for this and #288.

[srdanrasic]

Hey @AnthonyMDev, sorry for the slow progress during the past week. I moved to another country so I had very little time. Fortunately things are settling down now and I got some time during the weekend to work on this.

Please check out beta9.

KVO key path can now be represented using a DynamicSubject so it's bidirectional. Use method dynamic(keyPath:ofType:) to get it. It can be both observed and bound to.

In addition, deallocation handling is now improved and observation will be disposed just before the target object is deallocated. Observation does not need to hold a strong reference any more.

I still did not battle test this, but from extensive play in playgrounds there seems to be no issues. I'll be adding unit tests in following days. Please report if you find any issues.

If you really need a property, you can now do following:

public extension DynamicSubject {

  public func asProperty() -> Property<Element> {
    let property = Property<Element>(value)
    bidirectionalBind(to: property)
    return property
  }
}

[AnthonyMDev]

No apologies necessary at all! I know you're a busy guy and you work very hard to maintain this repository as quickly as possible. I just wanted to extend the offer of help again since I do need to get this for a release version ASAP.

These changes sound awesome! Thanks so much! That seems to fix #288, and it does fix the scenario where the object's deallocation was causing the fatalError. However, if the value fails to convert, will this still throw a fatalError? Have you added some other error handling for that, or did you decide to leave it as is?

Thanks again

[srdanrasic]

Ah, conversions... It will crash, but I prefer it that way because 1) you can always make it safer yourself and 2) it makes things easier to work with when everything is expected to be convertible - which I think is at least 90% of time. If the KVO signal would be failable, it would not be bindable.

If you expect KVO values that are not convertible, you can do conversion by yourself. Observe KVO values as Any? and then convert them manually.

let name: Signal<String, MyError> = object
  .dynamic(keyPath: "test", ofType: Optional<Any>.self)
  .tryMap { (value) -> Result<String, MyError> in
    if let value = value as? String {
      return .success(value)
    } else {
      return .failure(MyError.conversion)
    }
  }

Thank you for your assistance offer! Problem is that I wasn't sure myself how exactly should this be implement so I had to research a bit first.

[AnthonyMDev]

Cool thanks for the info!

[AnthonyMDev]

@srdanrasic I'm having trouble depending on the newest beta because it still uses a dependency on ReactiveKit beta-5. Can you release a new beta that uses ReactiveKit beta-6 please?

[srdanrasic]

@AnthonyMDev Just did!

[AnthonyMDev]

Thanks!

[AnthonyMDev]

@srdanrasic This is still causing me issues when dealing with NSManagedObject. The deallocation handling is working fine for the object itself, but often times the NSManagedObjectContext is deallocated before the object and it faulting the key values to nil. I don't really feel like the best option is to crash when the value is non-convertible by default.

[srdanrasic]

Damn. Let's make it a failable signal as default then. Give me a day, I'll have to refactor DynamicSubject to be failable too.

[AnthonyMDev]

@srdanrasic Well, I still understand your original argument that making it failable makes it harder to work with. So, we could consider something else...

What if, rather than crashing, no event was emitted if the value is not convertible?

If the KVO signal would be failable, it would not be bindable.

I want to be able to handle errors here, but I also don't think that its a good thing to make it so that DynamicSubject is no longer bindable. Bindable in ReactiveKit works with SignalProtocol even if it has an error. Why are you saying that it wouldn't be bindable?

[AnthonyMDev]

I saw that version 5.0.0 was released! Congrats! Did this not end up making the cut for this version?

[srdanrasic]

Thanks! Actually it did, I forgot to tell you :) This is how KVO interface looks now:

public extension NSObject {
  public func dynamic<T>(keyPath: String, ofType: T.Type) -> DynamicSubject<NSObject, T>
  public func dynamic<T>(keyPath: String, ofExpectedType: T.Type) -> DynamicSubject2<NSObject, T, KVOError>
}

If using second method, you get a failable signal (Signal<T, KVOError>) that will fire an error event KVOError.notConvertible when the value is not convertible. You'll have to handle and/or suppress the error before binding such signal using one of the ways mentioned in ReactiveKit docs. I guess suppressError(logging:) will be enough in your case.

[AnthonyMDev]

@srdanrasic This looks good, however this is now causing me to be unable to use bidirectionalBind on a DynamicSubject. Is that intended behavior? Is there a way to work around that?

Thanks!

[AnthonyMDev]

Never mind. I figured this out! Thanks so much!