-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
iOS 18 Support #23527
iOS 18 Support #23527
Conversation
e3bfbd8
to
51d0043
Compare
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
`cache` is actor-isolated, so it can’t be acessed from a `nonisolated` function without using `await` I just re-isolated it and updated the callers
9d2a4be
to
b7e09f5
Compare
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 made some more minor changes and updated the .xcode-version. The icons looks good, and everything else too.
@@ -486,6 +486,10 @@ extension UITableViewController: TableViewContainer {} | |||
|
|||
// MARK: - Diffable | |||
|
|||
// This conformance was added during the Xcode 16 migration to silence the | |||
// dozens of false-positive warnigns (any @unchecked conformance is tech debt). | |||
extension AnyHashable: @retroactive @unchecked Sendable {} |
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.
This line has a much bigger impact than the individual @checked Sendable
: all AnyHashable
instances now get Sendable
during runtime, which feels like an unsafe change...
I don't have any solution to solve the warning without changing how the two types below are used. Maybe we can tolerate the warnings for now until a proper ImmuTable
refactor?
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.
all AnyHashable instances now get Sendable during runtime, which feels like an unsafe change
It brings the app back to the status quo where in Xcode 15.4 where all AnyHashable
instances were also not producing any concurrency warnings.
Maybe we can tolerate the warnings for now until a proper ImmuTable refactor?
Unfortunately no because the vast majority of warnings are due to this type.
I don't have any solution to solve the warning without changing how the two types below are used.
The problem is two-fold:
UITableViewDiffableDataSource
requiringSendable
even when you never use it from the background (you can but very few people actually do). I think it's a pretty major limitation of the design of data race safety features in Swift 6.AnyHashable
can't becomeSendable
conditionally based on the type is is parameterized with because it erases this type at compile time.
The only fix seems to be to stop using AnyHashable
.
Co-authored-by: Tony Li <tony.li@automattic.com>
.buildkite/pipeline.yml
Outdated
@@ -79,7 +79,7 @@ steps: | |||
context: "UI Tests (iPhone)" | |||
|
|||
- label: "🔬 :jetpack: UI Tests (iPad)" | |||
command: .buildkite/commands/run-ui-tests.sh 'iPad Pro 13-inch (M4)' | |||
command: .buildkite/commands/run-ui-tests.sh 'iPad (10th generation) (17.5)' |
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 stopped trying to get it to work because I have another branch where I remove the .sidebar FF and with a fully updated set of tests.
let view = controller.navigationController?.view.subviews.first { view in | ||
return view is SuggestionsTableView | ||
} | ||
let suggestionsTableView = try XCTUnwrap(view) |
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 spent a good hour trying to debug this but because these tests don't follow best practices for unit testing and are evaluating the UIKit components, I made a call to remove them.
@@ -52,6 +52,13 @@ final class NotificationsViewControllerTests: XCTestCase { | |||
postAccountChangeNotification() | |||
|
|||
// Then | |||
// TODO: rework this unit test | |||
let expectation = self.expectation(description: "setBadgeCount") | |||
DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(100)) { |
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.
These tests are failing because the deprecated applicationIconBadgeNumber
property now works asynchronously. It's not great, but we can rework it when we remove the deprecated API.
a749182
to
a3778b7
Compare
a3778b7
to
d2410ee
Compare
Adds iOS 18 support – we're not building against that SDK yet, but right now this PR:
To test:
Ensure CI passes