-
Notifications
You must be signed in to change notification settings - Fork 73
fix: add deprecated overload for Observable based access #169
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @iwt-dahlborn, currently it is expected for you to get a perception warning if you are deploying to iOS 16 even if you are currently running on an iOS 17+ device. And the way we have things set up now is what allows that. Can you confirm that you were definitely receiving a perception warning for a project that was deploying to iOS 17+? |
|
Yes, the minimum deployment version of the app, and package the call were contained in, is iOS 17.0. Aditionally I was able to avoid the perception warning by switching to the non deprecated access call. |
|
I just may be misunderstanding the root issue, but can you describe why you were calling a deprecated method? Did something about the library force you into that situation? If this PR is only fixing something that only happens when invoking deprecated APIs, then I don't think we will be merging it. Deprecated APIs should be avoided and not sure we should introduce new surface area to help out those still using deprecated APIs. |
|
Basically I updated the tca version and migrated to observable but had not gotten around to update the deprecated call in this case. Before this function was deprecated it had 2 overloads: // Perceptible version
public func access<Subject: Perceptible, Member>(
_ subject: Subject,
keyPath: KeyPath<Subject, Member>,
fileID: StaticString = #fileID,
filePath: StaticString = #filePath,
line: UInt = #line,
column: UInt = #column
) {
// Observable version
public func access<Subject: Observable, Member>(
_ subject: Subject,
keyPath: KeyPath<Subject, Member>,
fileID: StaticString = #fileID,
filePath: StaticString = #filePath,
line: UInt = #line,
column: UInt = #column
) {But currently there exists only a deprecation for the The change in this pr practically ensures that the code works as expected for people who update the sdk and have not gotten around to update all the deprecation (which I know one should do). And I understand that you might not want to merge it since the fix for the user is to just update the code to the non-deprecated version of the function. Normally re-adding a function that was already removed as a deprecated would not make sense but since its an overloaded function that can under certain circumstances just "silently" (except for the deprecation warning) switches to the other overload it could help people who are updating to a new version of the package. So this would practically only be to ease the transition. Which also of course would only help if they update to this certain patch. If you think this is not a good idea we can discard this pr. Its not "needed" on my side since I simply removed the deprecated function calls to solve the issue. Edit: Small correction since its soft deprecated a deprecation warning for this call is not immediately visible in xcode causing extra confusion as to why it causes weird behavior. |
|
I created a small example project to visualise the "issue" when you open it you wont see any warning or errors in the code, but if you run it you get the perception warning asking you to add So since the function is only soft deprecated its not immediately obvious what you need to do to fix the perception warning, the only way to really find out is to attach the debugger and step through to hopefully see that the function is deprecated and understand that it links to the perceptible implementation instead of the observable one now. The additional deprecated function in this pr practically just allows the function to stay soft deprecated but still call the observable variant of the implementation. |
Currently if you call the perception registrar with a subject that is both
PerceptibleandObservableit will call theObservablevariants since thePerceptibleimplementations are disfavored.The exception is if your subject still calls the deprecated variant of the access function, in this case the
Perceptiblevariant will be called possibly leading to an erroneous perception runtime warning.Adding a
Observablebased implementation of the deprecated function reroutes the call to the non deprecatedObservableimplementation properly.I am not sure whether you want to integrate this since it practically adds a deprecated function and it can also be fixed on the user side by moving to the non-deprecated function. But the perception warning lead to some confusion in our project.