appsquickly / typhoon

Powerful dependency injection for Objective-C ✨✨ (https://PILGRIM.PH is the pure Swift successor to Typhoon!!)✨✨
https://pilgrim.ph
Apache License 2.0
2.7k stars 269 forks source link

Typhoon causes crash while transferring autolayout #513

Closed Brain89 closed 7 years ago

Brain89 commented 8 years ago

Hello,

We have faced with an issue crashing our app

Fatal Exception: NSUnknownKeyException [<NSLayoutConstraint 0x174283cf0> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key firstItem.

It appears only on devices running iOS 10 (yep, new SDK - new problems). I have attached crash log from Crashlytics. As I understand iOS 10 SDK does not support setter for firstItem property of NSLayoutConstraint.

CrashLog.txt

Przemyslaw-Wosko commented 8 years ago

how to reproduce it?

Brain89 commented 8 years ago

To reproduce this bug you should add TyphoonLoadedView into your project and run it inside iOS X environment.

vasilenkoigor commented 8 years ago

Hey @Brain89 Looks strange, because NSLayoutConstraint doesn't changed in iOS 10 SDK, with the exception of added firstAnchor and secondAnchor https://developer.apple.com/library/prerelease/content/releasenotes/General/iOS10APIDiffs/Objective-C/UIKit.html

However, I sure that crash happens in this place TyphoonViewHelpers

[constraint setValue:firstItem forKey:NSStringFromSelector(@selector(firstItem))];

Might be create issue on radar? I'm think this is SDK bug.

Brain89 commented 8 years ago

I do not agree that this is a SDK bug. firstItem is a readonly property (see docs)

@property(readonly, assign) id firstItem

So the code to which you refer uses private API. Apple engineers could change it without any notice.

vasilenkoigor commented 8 years ago

@Brain89 hmm...but wait, this property is readonly since iOS 6... nope? :)

vasilenkoigor commented 8 years ago

May be Apple engineers closed back-door that allows change readonly property with KVC😁 Ok, in few days I'll make workaround.

Thanks for reporting👌

Brain89 commented 8 years ago

We must reopen this issue. Merged PR has broken https://github.com/appsquickly/Typhoon/pull/464 @CognitiveDisson will fix it.

alexgarbarev commented 8 years ago

@Brain89 but how do you think this can be fixed without using "setValue:forKey" since it crashes on iOS10 that way?

CognitiveDisson commented 8 years ago

@alexgarbarev Subclass of NSLayoutConstraint fixed this issue. If constraint is it subclass then transfer, else create new constraint. What do you think about this solution?

alexgarbarev commented 8 years ago

So user have to choose subclass in IB while creating a constraint? For me it looks invasive. It looks like there is no legal way to modify existing constraint's first and last items.. Maybe it's better to not include support for outlets for these constraints in Typhoon, then it's more stable since uses legal APIs. Anyone who wants TyphoonLoadedView with constraints and outlet always free to use own solution with subclasses, swizzling or anything else.. @vasilenkoigor what do you think about this problem?

Brain89 commented 8 years ago

Well, without creating NSLayoutContraint subclass inside library, every Typhoon user with constraint outlets necessity (like in https://github.com/appsquickly/Typhoon/pull/464) will have to create his own Typhoon fork, where it will be possible to fix this issue (create subclass and improve ViewHelpers in a way @CognitiveDisson wants). Seems very strange too.

CognitiveDisson commented 8 years ago

@alexgarbarev If user not use IB to TyphoonLoadedView, then he does not need set subclass. (And all be works like now) But if the user will need this functionality, it will only need to specify the subclass. Another solution is swizzling. (and this solution even more worse)

alexgarbarev commented 8 years ago

ok, let's see solution with subclass.

etolstoy commented 7 years ago

Fixed in #524