Skip to content
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

Make package safe for Swift 6 #50

Merged
merged 4 commits into from
Nov 9, 2024

Conversation

ptoffy
Copy link
Contributor

@ptoffy ptoffy commented Nov 8, 2024

So after #48 and the introduction of Swift 6, some @unchecked Sendable annotations were added. While this compiles, it's not the correct way to approach the safety concerns introduced by Swift 6. This PR attempts to fix that by (over?)annotating the package with Sendable. I think this is a thing that should be done before releasing the latest major version as we're free to make breaking changes.
The PR is currently in a semi-finished state as it should work as it is, but it's difficult to tell without tests. I also think that some improvements can be made to this solution as we do still have a few classes and mutable state around but I'm open to discussions about it

Also adds a build workflow for main and PRs

Sources/FCM/Helpers/FCM+SendMessage.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Sources/FCM/FCMAndroidConfig/FCMAndroidNotification.swift Outdated Show resolved Hide resolved
@ptoffy ptoffy requested a review from MihaelIsaev November 8, 2024 15:19
@ptoffy
Copy link
Contributor Author

ptoffy commented Nov 8, 2024

@MihaelIsaev there's a batchSend method in the README I can't find in the repo. I don't need it but if it's been removed it might make sense to remove it from the README too

@MihaelIsaev
Copy link
Owner

@ptoffy Yeah it's been removed in #48 according to #45 so yeah it should be removed from the README as well

@MihaelIsaev
Copy link
Owner

@ptoffy Have you tried it in action with Swift 6? I don’t have a chance to test it right now, but if everything works well, I’m ready to merge and tag it as v3.

@MihaelIsaev MihaelIsaev merged commit 0c5f3c8 into MihaelIsaev:master Nov 9, 2024
@MihaelIsaev
Copy link
Owner

@ptoffy Have you tried it in action with Swift 6?

@ptoffy
Copy link
Contributor Author

ptoffy commented Nov 11, 2024

Sorry @MihaelIsaev missed this. I have barely tried it locally and yet have to try it in a staging/prod environment, it will likely be this week. I'd suggest tagging a beta for now

@MihaelIsaev
Copy link
Owner

@ptoffy Thank you very much for your contribution!
It’s now available in the 3.0.0-beta.1 tag.
Can’t wait for your feedback from tests in staging/prod environments ⚡️

@ptoffy ptoffy deleted the swift6-safe branch November 18, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants