[2.x] Add backward compatibility for callback API changes #684
Conversation
| * Swift/Objective-C methods!!! | ||
| */ | ||
| @property(atomic, readwrite, assign, nullable) KSReportWriteCallback onCrash; | ||
| @property(atomic, readwrite, assign, nullable) KSReportWriteCallback onCrash |
There was a problem hiding this comment.
We had a type mismatch. The typedef KSReportWriteCallback was declared outside NS_ASSUME_NONNULL_BEGIN, making the writer parameter nullable. But in KSCrashConfiguration, the same callback was declared as void (^crashNotifyCallback)(const struct KSCrashReportWriter *writer) inside NS_ASSUME_NONNULL_BEGIN, making the writer parameter non-null by default. We have to restore that API as it blows up the compilation.
There was a problem hiding this comment.
We have to restore that as well, as nonNullable writer breaks the API as well
There was a problem hiding this comment.
Actually it's not good that a type from the underlying module that we consider "private API" is used in public API.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR adds backward compatibility for callback API changes introduced in PR #676 by restoring the original callback signatures while maintaining the enhanced functionality with new policy-aware callbacks.
- Introduces new callback types with "WithPolicy" suffix that include policy parameter for async-safety awareness
- Maintains legacy callbacks with deprecation warnings to prevent compilation breaks
- Implements adapter functions to bridge old callbacks to the new system
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/KSCrashRecordingTests/KSCrashConfiguration_Tests.m | Updates test references to use new "WithPolicy" callback properties |
| Sources/KSCrashRecordingCore/include/KSCrashExceptionHandlingPolicy.h | Adds CoreFoundation import and Swift naming for policy struct |
| Sources/KSCrashRecording/include/KSCrashReportWriterCallbacks.h | New header defining policy-aware callback types |
| Sources/KSCrashRecording/include/KSCrashReportWriter.h | Removes nullability annotations and adds deprecated legacy callback types |
| Sources/KSCrashRecording/include/KSCrashConfiguration.h | Adds deprecated legacy callback properties alongside new policy-aware ones |
| Sources/KSCrashRecording/include/KSCrashCConfiguration.h | Adds deprecated legacy callback fields in C configuration struct |
| Sources/KSCrashRecording/KSCrashReportC.h | Updates function signature to use policy-aware callback |
| Sources/KSCrashRecording/KSCrashReportC.c | Updates internal callback type to use policy-aware version |
| Sources/KSCrashRecording/KSCrashConfiguration.m | Implements bridge logic for converting legacy callbacks to new system |
| Sources/KSCrashRecording/KSCrashC.c | Implements adapter functions and callback selection logic for backward compatibility |
| Sources/KSCrashInstallations/include/KSCrashInstallation.h | Adds deprecated legacy callback property alongside new policy-aware one |
| Sources/KSCrashInstallations/KSCrashInstallation.m | Implements backward compatibility handling in installation callback logic |
| Samples/Common/Sources/LibraryBridge/InstallBridge.swift | Updates sample code to use new policy-aware callback properties |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
Hmm maybe we need a user-facing policy struct? We'd have to remember to keep it in sync with the internal one, though... |
Let’s do it in another PR. In this I just want to get project back on track |
…precate legacy callbacks
… and fix test deprecation warnings
… legacy definitions
2b4dc12 to
17605d7
Compare
|
I think I’ll set up this tool https://github.com/Adyen/adyen-swift-public-api-diff before the release, since I’m not fully confident that nothing got broken after multiple API changes across different PRs. It also comes with a convenient CI action. Hopefully it will work with our API as well. For now, I believe checking the Swift API should be enough, as it’s the most sensitive to all the nullability issues. |
…new policies over legacy implementations
Summary
This PR fixes the breaking changes from PR #676 by adding backward compatibility for the crash reporting callback APIs.
Background
PR #676 broke existing code by changing the callback signatures to include a policy parameter. Users upgrading would suddenly get compilation errors.
What I Changed
Brought back the original callbacks
KSReportWriteCallbackandKSReportWrittenCallbacksignaturesAdded deprecation warnings
Made migration smooth
How to Migrate
Current code (still works, just shows warnings):
Updated code: