-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fallback to Perception on iOS 17 beta builds #66
Fallback to Perception on iOS 17 beta builds #66
Conversation
…umers to force perception checking
+1 |
@ollieatkinson Thanks for this. I'm doing the revert and release separately here: #67. I think we need to do more thinking. While we'd love to avoid these crashes, it also doesn't seem like we should create too much library uncertainty and problems to accommodate beta users. I wonder if there is a more targeted fix that could be done? Maybe a way of detecting that the user has a beta OS that is affected and avoid the cast? Also I think we need a bit more of a write-up in this PR to consider adding the property. I haven't fully wrapped my head around what it does :) |
The purpose of the property is to force the use of |
@ollieatkinson If we go this route I think documentation that explains why a user may enable this flag would be helpful, and example code of how/where to do so. But also I wonder if there's a way to do this in a more targeted fashion that doesn't require configuration? Could this boolean be detected based off an OS check at runtime? |
Yes, totally - I understand the hesitation. We would need to use
My original intention for adding this boolean was to provide a configuration escape hatch for consumers if they ever found another issue and this helped them work around it. Please let me know if you would prefer a runtime |
e.g. import Foundation
#if os(iOS) || os(tvOS)
import UIKit
#elseif os(watchOS)
import WatchKit
#endif
var forcePerceptionChecking: Bool { isBeta && isSystemVersionObservationBeta }
private var isBeta: Bool { kernelVersion.last?.isLowercase == true }
private var isSystemVersionObservationBeta: Bool {
#if os(iOS) || os(tvOS)
return UIDevice.current.systemVersion == "17.0"
#elseif os(watchOS)
return WKInterfaceDevice.current().systemVersion == "10.0"
#elseif os(macOS)
return false
#endif
}
private var kernelVersion: String {
var size = 0
sysctlbyname("kern.osversion", nil, &size, nil, 0)
var version = [CChar](repeating: 0, count: size)
sysctlbyname("kern.osversion", &version, &size, nil, 0)
return String(cString: version)
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for figuring out the fix! Made a few small changes and I think we're good to merge!
- Changed the computed
var
s to alet
so the logic will only run once - Moved the
sysctlbyname
calls into the#if
s where it's needed - Pruned the comments a bit
@ollieatkinson Before merging, can you confirm that pointing your project to this branch works fine for you? Everything building/compiling and working as expected, even for the beta bucket? I'm hoping even things like |
Everything is building compiling as expected, the one caveat is that I have been unable to test this since When running on my physical device I can confirm the 17.4.1: https://betawiki.net/wiki/IOS_17.4.1_build_21E237
Beta: https://betawiki.net/wiki/IOS_17.0_build_21A5277j If you are happy to move forward with this assumption I would be too - if not, I can look to try ship this branch into a release and await for customer feedback. |
@ollieatkinson Huh! In the above code sample does And since this fix was brought up because of your user base, I think it'd definitely be a good idea to roll it out to you users and verify the fix before merging. Assuming everything looks good on your end after the rollout, we'll be happy to merge! |
From limited testing, yes, however I would not want to rely on this behaviour.
Unfortunately not, the documentation for this property states "This string is not appropriate for parsing."
This makes sense, let me go ahead to make this change and report back |
Hey @stephencelis - looping back on this, since merging and having the fix in customers hands for ~3 weeks we have not observed any crashes! |
Early beta versions used the
@_marker
protocol for Observable.Changes to the protocol's memory layout in these versions disrupt dynamic casting,
e.g.
subject as? any Observable
. This occurs because the Swift runtime expects a specificprotocol metadata layout for casting, and this mismatch leads to a crash.
As a result, the following check has been implemented to disable Observation on any of the
following beta OS versions:
This safeguard makes sure that
Perception
is used in place ofObservation
.