Replacing NSCoder with Swift's Codable #868
Conversation
Vaidios
changed the title
Auth0/CredentialsManager.swift
Outdated
| do { | ||
| return try JSONDecoder().decode(Credentials.self, from: data) | ||
| } catch { | ||
| print(error.localizedDescription) |
There was a problem hiding this comment.
@Vaidios I know that this error was already not being surfaced here, so this is not a comment on your PR but more this process in general. If an NSCoder failure is the cause of #862 there is an argument to be made that the problem would have been easier to debug had this function either throw here and bubbled the error up or have the .noCredentials error provide a more descriptive message. What are your thoughts @desusai7 ?
There was a problem hiding this comment.
I totally agree, it would be much easier to find a root cause if that error would be propagated.
Maybe we should throw an error such as 'credentialsParsingFailure' instead of non-descriptive 'noCredentials'.
PS. For the time being, I will remove that do-catch with print to be optional try, just as before
Vaidios
force-pushed
the
removing-nskeyedunarchiver
branch
from
42deca3
to
6b18d73
Compare
|
@theolampert sorry for tagging you here, I was just wondering if there is any button to ask for the review from maintainers and I clicked it without thinking much about it :E |
|
Hi @Vaidios, Thank you for raising a PR to address this, we will look into it and get back to you ! |
|
Hey @desusai7, are there any updates on this? |
📋 Changes
📎 References
It doesn't seem like usage of this API is required in this context. Credentials type doesn't use polymorphism, which could be only handled by NSKeyedUnarchiver.
Thanks to these changes my code now properly runs inside a SPM package
Please let me know if there is something that I don't understand :)
https://forums.swift.org/t/should-i-stick-with-codable-or-switch-back-to-nscoding/61604
#862
🎯 Testing
This is not a breaking change, these code paths are covered by unit tests