refactor: move EventHandlers to CheckoutSheetKit and simplify event handling #336
Conversation
…rameters - Update onComplete to pass CheckoutCompletedEvent with order details - Update onFail to pass CheckoutError for better error handling - Rename ApplePay controller properties for clarity (e.g., onComplete → onCheckoutComplete) - Remove ShopifyCheckoutSheetKit namespace prefixes for cleaner code - Ensure thread safety with proper MainActor usage in delegate methods - Update all tests to match new event handler signatures
- Add event parameter to onComplete callback to access order details - Add error parameter to onFail callback for better error logging - Update onWebPixelEvent to extract and display only the event name - Import ShopifyCheckoutSheetKit for access to event types
…andling - Move EventHandlers struct from ShopifyAcceleratedCheckouts to ShopifyCheckoutSheetKit - Refactor CheckoutSheet to use EventHandlers directly instead of delegate wrapper - Update CheckoutWebViewController to support both delegate and EventHandlers patterns - Add new onShouldRecoverFromError method to CheckoutSheet - Update tests to work with new EventHandlers approach
| @discardableResult public func onShouldRecoverFromError(_ action: @escaping (CheckoutError) -> Bool) -> Self { | ||
| var newView = self | ||
| newView.eventHandlers.shouldRecoverFromError = action | ||
| return newView | ||
| } | ||
| } | ||
|
|
||
| public class CheckoutDelegateWrapper: CheckoutDelegate { |
There was a problem hiding this comment.
With the changes on this PR we are supporting 2 patterns: the CheckoutDelegate protocol and the EventHandlers struct. We could simplify this through the following changes:
1. Unify on EventHandlers Pattern
Remove the delegate protocol entirely and use only EventHandlers:
// Before: Two patterns
init(checkoutURL: URL, delegate: CheckoutDelegate?)
init(checkoutURL: URL, eventHandlers: EventHandlers)
// After: Single pattern
init(checkoutURL: URL, eventHandlers: EventHandlers = EventHandlers())2. Eliminate CheckoutDelegateWrapper
This class only exists to convert between patterns and adds no value.
3. Direct Event Propagation
Remove intermediate delegation:
// Current: Multiple hops
CheckoutWebView → CheckoutWebViewController → delegate → wrapper → handlers
// Simplified: Direct propagation
CheckoutWebView → eventHandlers
4. Consolidate Thread Safety
Instead of wrapping each callback in Task { @mainactor in ... }, handle this once in EventHandlers:
public struct EventHandlers {
// Wrap closures to ensure MainActor execution
public init(
checkoutDidComplete: ((CheckoutCompletedEvent) -> Void)? = nil,
// ... other handlers
) {
self.checkoutDidComplete = checkoutDidComplete.map { handler in
{ event in
Task { @MainActor in handler(event) }
}
}
// ... wrap other handlers similarly
}
}
There was a problem hiding this comment.
Still re-familiarising myself with all of this code but here are my thoughts so far from looking through it.
We use a delegate pattern currently for the programmatic usage of the sheet. i.e. .present(url, someDelegate). The delegate is a protocol, so classes must conform to it.
EventHandlers is almost identical to CheckoutDelegate except it is a Struct instead of a Protocol and it doesn't offer defaults out of the box like the CheckoutDelegate does.
Remove the delegate protocol entirely and use only EventHandlers:
Why would we favour this when it diverges from the programmatic usage. Wouldn't it make more sense to align the implementation here with the existing delegate pattern?
There was a problem hiding this comment.
I think the solution to this should probably be that EventHandlers is a class that conforms to CheckoutDelegate (might want to be observable for the modifier changes to apply)
@Observable class EventHandlers : CheckoutDelegate {
// implementation remains the same
}By doing this the overloads in CheckoutViewController.swift to accept CheckoutDelegate or EventHandlers is no longer necessary, we just request have the CheckoutDelegate initializer
EventHandlers was intended to be a grouping of the CheckoutDelegate handlers to avoid prop drilling ~5 arguments (and more as we support more in future).
I don't think we should remove CheckoutDelegate in favour of EventHandlers. The protocol is idiomatic swift and provides flexibility in implementation, as it allows consumers to conform their existing classes to implement as CheckoutDelegates without defining the "structure".
There was a problem hiding this comment.
Agree @kieran-osgood-shopify, though how would the current CheckoutDelegateWrapper differ from the EventHandlers class you're proposing here?
There was a problem hiding this comment.
Ahhh, I didn't realise we already had a class for this, yeah that class would work for us
| OSLogger.shared.debug( | ||
| "[CheckoutViewController#updateUIViewController]: No ViewControllers matching CheckoutWebViewController \(uiViewController.viewControllers.map { String(describing: $0.self) }.joined(separator: ""))" | ||
| ) | ||
| return | ||
| } | ||
| OSLogger.shared.debug( | ||
| "[CheckoutViewController#updateUIViewController]: No ViewControllers matching CheckoutWebViewController \(uiViewController.viewControllers.map { String(describing: $0.self) }.joined(separator: ""))" | ||
| ) |
There was a problem hiding this comment.
This log has been moved to the else branch because it states No ViewControllers but was running when there was a controller.
josemiguel-alvarez
force-pushed
the
jm/update-accelerated-checkouts-event-handlers
branch
from
aa537f4 to
a9d307c
Compare
| init(checkout url: URL, eventHandlers: EventHandlers) { | ||
| let rootViewController = CheckoutWebViewController(checkoutURL: url, eventHandlers: eventHandlers) | ||
| rootViewController.notifyPresented() | ||
| super.init(rootViewController: rootViewController) | ||
| presentationController?.delegate = rootViewController | ||
| } |
There was a problem hiding this comment.
If I understand correctly, this overloads the class to allow both delegate and eventHandlers. Is this for backwards compatibility sake?
There was a problem hiding this comment.
Yes, it's what I wanted to highlight on this comment. There I'm proposing ways to simplify it.
This PR moves EventHandlers to CheckoutSheetKit and simplify event handling.
What changes are you making?