-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add compatibility for iOS 15 and friends #181
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
=======================================
Coverage 78.76% 78.76%
=======================================
Files 60 60
Lines 1413 1413
=======================================
Hits 1113 1113
Misses 300 300 |
@@ -5,9 +5,9 @@ let package = Package( | |||
name: "jwt-kit", | |||
platforms: [ | |||
.macOS(.v13), |
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.
Do we want to bump this to 12 to line up?
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.
There's a few errors around EC keys about missing conformances with .macOS
< 13. Also JSONDecoder
and JSONEncoder
are only Sendable
in 13+
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.
I mean we can try solving them but not sure it's worth it?
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.
What are the issues with EC keys? Because surely we'll have the same issues on iOS 15
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.
I wonder if it's worth being really annoying and adding CI for iOS (including 15) so see what issues we'd hit
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.
The thing is Crypto doesn't specify iOS requirements (or any besides macOS) so there's no issues with that
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! Looks good to me. I'd recommend lowering this as much as feasible, going even back to iOS 13 if possible. You can always use @available
annotations if need be for specific features.
Fix #180