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

Ensure thread safety #32

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions Source/Bridge.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ protocol Bridgable: AnyObject {
/// `Bridge` is the object for configuring a web view and
/// the channel for sending/receiving messages
public final class Bridge: Bridgable {
public typealias InitializationCompletionHandler = () -> Void
weak var delegate: BridgeDelegate?
weak var webView: WKWebView?

Expand All @@ -26,38 +27,38 @@ public final class Bridge: Bridgable {
initialize(Bridge(webView: webView))
}
}

init(webView: WKWebView) {
self.webView = webView
loadIntoWebView()
}

deinit {
webView?.configuration.userContentController.removeScriptMessageHandler(forName: scriptHandlerName)
}

svara marked this conversation as resolved.
Show resolved Hide resolved
// MARK: - Internal API

/// Register a single component
/// - Parameter component: Name of a component to register support for
@MainActor
func register(component: String) async throws {
try await callBridgeFunction(.register, arguments: [component])
}

/// Register multiple components
/// - Parameter components: Array of component names to register
@MainActor
func register(components: [String]) async throws {
try await callBridgeFunction(.register, arguments: [components])
}

/// Unregister support for a single component
/// - Parameter component: Component name
@MainActor
func unregister(component: String) async throws {
try await callBridgeFunction(.unregister, arguments: [component])
}

/// Send a message through the bridge to the web application
/// - Parameter message: Message to send
@MainActor
func reply(with message: Message) async throws {
logger.debug("bridgeWillReplyWithMessage: \(String(describing: message))")
let internalMessage = InternalMessage(from: message)
Expand All @@ -72,15 +73,15 @@ public final class Bridge: Bridgable {
// let replyMessage = message.replacing(data: data)
// callBridgeFunction("send", arguments: [replyMessage.toJSON()])
// }
@MainActor
@discardableResult
func evaluate(javaScript: String) async throws -> Any {
@MainActor
func evaluate(javaScript: String) async throws -> Any? {
guard let webView else {
throw BridgeError.missingWebView
}

do {
return try await webView.evaluateJavaScript(javaScript)
return try await webView.evaluateJavaScriptAsync(javaScript)
} catch {
Comment on lines 83 to 85
Copy link
Collaborator Author

@svara svara Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using async/await version of evaluateJavaScript, WKWebView expects JavaScript to return with a value (something other than Void). If there's no value returning from the JavaScript that you evaluate it will crash because it tries to unwrap a nil value to Any.

logger.error("Error evaluating JavaScript: \(error)")
throw error
Expand All @@ -90,7 +91,8 @@ public final class Bridge: Bridgable {
/// Evaluates a JavaScript function with optional arguments by encoding the arguments
/// Function should not include the parens
/// Usage: evaluate(function: "console.log", arguments: ["test"])
func evaluate(function: String, arguments: [Any] = []) async throws -> Any {
@MainActor
func evaluate(function: String, arguments: [Any] = []) async throws -> Any? {
try await evaluate(javaScript: JavaScript(functionName: function, arguments: arguments).toString())
}

Expand All @@ -112,6 +114,7 @@ public final class Bridge: Bridgable {
/// The webkit.messageHandlers name
private let scriptHandlerName = "strada"

@MainActor
private func callBridgeFunction(_ function: JavaScriptBridgeFunction, arguments: [Any]) async throws {
let js = JavaScript(object: bridgeGlobal, functionName: function.rawValue, arguments: arguments)
try await evaluate(javaScript: js)
Expand Down Expand Up @@ -151,7 +154,8 @@ public final class Bridge: Bridgable {
// MARK: - JavaScript Evaluation

@discardableResult
private func evaluate(javaScript: JavaScript) async throws -> Any {
@MainActor
private func evaluate(javaScript: JavaScript) async throws -> Any? {
do {
return try await evaluate(javaScript: javaScript.toString())
} catch {
Expand All @@ -169,9 +173,9 @@ public final class Bridge: Bridgable {

extension Bridge: ScriptMessageHandlerDelegate {
@MainActor
func scriptMessageHandlerDidReceiveMessage(_ scriptMessage: WKScriptMessage) async throws {
func scriptMessageHandlerDidReceiveMessage(_ scriptMessage: WKScriptMessage) {
if let event = scriptMessage.body as? String, event == "ready" {
try await delegate?.bridgeDidInitialize()
delegate?.bridgeDidInitialize()
return
}

Expand All @@ -183,3 +187,22 @@ extension Bridge: ScriptMessageHandlerDelegate {
logger.warning("Unhandled message received: \(String(describing: scriptMessage.body))")
}
}

private extension WKWebView {
/// NOTE: The async version crashes the app with `Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value`
/// in case the function doesn't return anything.
/// This is a workaround. See https://forums.developer.apple.com/forums/thread/701553 for more details.
@discardableResult
@MainActor
func evaluateJavaScriptAsync(_ javaScriptString: String) async throws -> Any? {
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Any?, Error>) in
evaluateJavaScript(javaScriptString) { data, error in
if let error {
continuation.resume(throwing: error)
} else {
continuation.resume(returning: data)
}
}
}
}
}
7 changes: 5 additions & 2 deletions Source/BridgeComponent.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation

@MainActor
protocol BridgingComponent: AnyObject {
static var name: String { get }
var delegate: BridgingDelegate { get }
Expand All @@ -22,6 +23,7 @@ protocol BridgingComponent: AnyObject {
func viewDidDisappear()
}

@MainActor
open class BridgeComponent: BridgingComponent {
public typealias ReplyCompletionHandler = (Result<Bool, Error>) -> Void

Expand All @@ -30,7 +32,7 @@ open class BridgeComponent: BridgingComponent {
/// Subclasses must provide their own implementation of this property.
///
/// - Note: This property is used for identifying the component.
open class var name: String {
nonisolated open class var name: String {
fatalError("BridgeComponent subclass must provide a unique 'name'")
}

Expand Down Expand Up @@ -149,7 +151,7 @@ open class BridgeComponent: BridgingComponent {

@discardableResult
/// Replies to the web with the last received message for a given `event`, replacing its `jsonData`
/// with the provided `Encodable` object.
/// with the provided `Encodable` object.
///
/// NOTE: If a message has not been received for the given `event`, the reply will be ignored.
///
Expand Down Expand Up @@ -275,3 +277,4 @@ open class BridgeComponent: BridgingComponent {

private var receivedMessages = [String: Message]()
}

15 changes: 11 additions & 4 deletions Source/BridgeDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import WebKit

public protocol BridgeDestination: AnyObject {}

@MainActor
public protocol BridgingDelegate: AnyObject {
var location: String { get }
var destination: BridgeDestination { get }
Expand All @@ -20,10 +21,11 @@ public protocol BridgingDelegate: AnyObject {

func component<C: BridgeComponent>() -> C?

func bridgeDidInitialize() async throws
func bridgeDidInitialize()
func bridgeDidReceiveMessage(_ message: Message) -> Bool
}

@MainActor
public final class BridgeDelegate: BridgingDelegate {
public let location: String
public unowned let destination: BridgeDestination
Expand Down Expand Up @@ -109,9 +111,15 @@ public final class BridgeDelegate: BridgingDelegate {

// MARK: Internal use

public func bridgeDidInitialize() async throws {
public func bridgeDidInitialize() {
let componentNames = componentTypes.map { $0.name }
try await bridge?.register(components: componentNames)
Task {
do {
try await bridge?.register(components: componentNames)
} catch {
logger.error("bridgeDidFailToRegisterComponents: \(error)")
}
}
}

@discardableResult
Expand Down Expand Up @@ -153,4 +161,3 @@ public final class BridgeDelegate: BridgingDelegate {
return component
}
}

4 changes: 2 additions & 2 deletions Source/ScriptMessageHandler.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import WebKit

protocol ScriptMessageHandlerDelegate: AnyObject {
func scriptMessageHandlerDidReceiveMessage(_ scriptMessage: WKScriptMessage) async throws
func scriptMessageHandlerDidReceiveMessage(_ scriptMessage: WKScriptMessage)
}

// Avoids retain cycle caused by WKUserContentController
Expand All @@ -13,6 +13,6 @@ final class ScriptMessageHandler: NSObject, WKScriptMessageHandler {
}

func userContentController(_ userContentController: WKUserContentController, didReceive scriptMessage: WKScriptMessage) {
Task { try await delegate?.scriptMessageHandlerDidReceiveMessage(scriptMessage) }
delegate?.scriptMessageHandlerDidReceiveMessage(scriptMessage)
}
}
12 changes: 12 additions & 0 deletions Strada.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
E2DB15912A7163B0001EE08C /* BridgeDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2DB15902A7163B0001EE08C /* BridgeDelegate.swift */; };
E2DB15932A7282CF001EE08C /* BridgeComponent.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2DB15922A7282CF001EE08C /* BridgeComponent.swift */; };
E2DB15952A72B0A8001EE08C /* BridgeDelegateTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2DB15942A72B0A8001EE08C /* BridgeDelegateTests.swift */; };
E2F4E06B2B9095BC000A3A24 /* TimeInterval+ExpectationTimeout.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2F4E06A2B9095BC000A3A24 /* TimeInterval+ExpectationTimeout.swift */; };
E2FDCF982A8297DA003D27AE /* BridgeComponentTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2FDCF972A8297DA003D27AE /* BridgeComponentTests.swift */; };
E2FDCF9B2A829AEE003D27AE /* BridgeSpy.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2FDCF9A2A829AEE003D27AE /* BridgeSpy.swift */; };
E2FDCF9D2A829C6F003D27AE /* TestData.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2FDCF9C2A829C6F003D27AE /* TestData.swift */; };
Expand Down Expand Up @@ -79,6 +80,7 @@
E2DB15902A7163B0001EE08C /* BridgeDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BridgeDelegate.swift; sourceTree = "<group>"; };
E2DB15922A7282CF001EE08C /* BridgeComponent.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BridgeComponent.swift; sourceTree = "<group>"; };
E2DB15942A72B0A8001EE08C /* BridgeDelegateTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BridgeDelegateTests.swift; sourceTree = "<group>"; };
E2F4E06A2B9095BC000A3A24 /* TimeInterval+ExpectationTimeout.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TimeInterval+ExpectationTimeout.swift"; sourceTree = "<group>"; };
E2FDCF972A8297DA003D27AE /* BridgeComponentTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BridgeComponentTests.swift; sourceTree = "<group>"; };
E2FDCF9A2A829AEE003D27AE /* BridgeSpy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BridgeSpy.swift; sourceTree = "<group>"; };
E2FDCF9C2A829C6F003D27AE /* TestData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestData.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -146,6 +148,7 @@
9274F1F22229963B003E85F4 /* Tests */ = {
isa = PBXGroup;
children = (
E2F4E0692B9095A5000A3A24 /* Extensions */,
E227FAF12A94D48C00A645E4 /* ComponentTestExample */,
E2FDCF9C2A829C6F003D27AE /* TestData.swift */,
E2FDCF992A829AD5003D27AE /* Spies */,
Expand Down Expand Up @@ -181,6 +184,14 @@
path = ComponentTestExample;
sourceTree = "<group>";
};
E2F4E0692B9095A5000A3A24 /* Extensions */ = {
isa = PBXGroup;
children = (
E2F4E06A2B9095BC000A3A24 /* TimeInterval+ExpectationTimeout.swift */,
);
path = Extensions;
sourceTree = "<group>";
};
E2FDCF992A829AD5003D27AE /* Spies */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -327,6 +338,7 @@
files = (
E2DB15952A72B0A8001EE08C /* BridgeDelegateTests.swift in Sources */,
E227FAF02A94D34E00A645E4 /* ComposerComponent.swift in Sources */,
E2F4E06B2B9095BC000A3A24 /* TimeInterval+ExpectationTimeout.swift in Sources */,
E227FAEE2A94B35900A645E4 /* BridgeDelegateSpy.swift in Sources */,
C11349C2258801F6000A6E56 /* JavaScriptTests.swift in Sources */,
E227FAF32A94D57300A645E4 /* ComposerComponentTests.swift in Sources */,
Expand Down
5 changes: 1 addition & 4 deletions Tests/BridgeComponentTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import XCTest
import WebKit
import Strada

@MainActor
class BridgeComponentTest: XCTestCase {
private var delegate: BridgeDelegateSpy!
private var destination: AppBridgeDestination!
Expand Down Expand Up @@ -222,7 +223,3 @@ class BridgeComponentTest: XCTestCase {
wait(for: [expectation], timeout: .expectationTimeout)
}
}

private extension TimeInterval {
static let expectationTimeout: TimeInterval = 5
}
8 changes: 6 additions & 2 deletions Tests/BridgeDelegateTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import XCTest
import WebKit
@testable import Strada

@MainActor
class BridgeDelegateTests: XCTestCase {
private var delegate: BridgeDelegate!
private var destination: AppBridgeDestination!
Expand All @@ -23,8 +24,11 @@ class BridgeDelegateTests: XCTestCase {
}

func testBridgeDidInitialize() async throws {
try await delegate.bridgeDidInitialize()

await withCheckedContinuation { continuation in
bridge.registerComponentsContinuation = continuation
delegate.bridgeDidInitialize()
}

XCTAssertTrue(bridge.registerComponentsWasCalled)
XCTAssertEqual(bridge.registerComponentsArg, ["one", "two"])

Expand Down
8 changes: 1 addition & 7 deletions Tests/BridgeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import XCTest
import WebKit
@testable import Strada

@MainActor
class BridgeTests: XCTestCase {
func testInitWithANewWebViewAutomaticallyLoadsIntoWebView() {
let webView = WKWebView()
Expand All @@ -27,7 +28,6 @@ class BridgeTests: XCTestCase {
/// NOTE: Each call to `webView.evaluateJavaScript(String)` will throw an error.
/// We intentionally disregard any thrown errors (`try? await bridge...`)
/// because we validate the evaluated JavaScript string ourselves.
@MainActor
func testRegisterComponentCallsJavaScriptFunction() async throws {
let webView = TestWebView()
let bridge = Bridge(webView: webView)
Expand All @@ -37,7 +37,6 @@ class BridgeTests: XCTestCase {
XCTAssertEqual(webView.lastEvaluatedJavaScript, "window.nativeBridge.register(\"test\")")
}

@MainActor
func testRegisterComponentsCallsJavaScriptFunction() async throws {
let webView = TestWebView()
let bridge = Bridge(webView: webView)
Expand All @@ -47,7 +46,6 @@ class BridgeTests: XCTestCase {
XCTAssertEqual(webView.lastEvaluatedJavaScript, "window.nativeBridge.register([\"one\",\"two\"])")
}

@MainActor
func testUnregisterComponentCallsJavaScriptFunction() async throws {
let webView = TestWebView()
let bridge = Bridge(webView: webView)
Expand All @@ -57,7 +55,6 @@ class BridgeTests: XCTestCase {
XCTAssertEqual(webView.lastEvaluatedJavaScript, "window.nativeBridge.unregister(\"test\")")
}

@MainActor
func testSendCallsJavaScriptFunction() async throws {
let webView = TestWebView()
let bridge = Bridge(webView: webView)
Expand All @@ -78,7 +75,6 @@ class BridgeTests: XCTestCase {
XCTAssertEqual(webView.lastEvaluatedJavaScript, "window.nativeBridge.replyWith({\"component\":\"page\",\"event\":\"connect\",\"data\":{\"title\":\"Page-title\"},\"id\":\"1\"})")
}

@MainActor
func testEvaluateJavaScript() async throws {
let webView = TestWebView()
let bridge = Bridge(webView: webView)
Expand All @@ -88,7 +84,6 @@ class BridgeTests: XCTestCase {
XCTAssertEqual(webView.lastEvaluatedJavaScript, "test(1,2,3)")
}

@MainActor
func testEvaluateJavaScriptReturnsErrorForNoWebView() async throws {
let bridge = Bridge(webView: WKWebView())
bridge.webView = nil
Expand All @@ -101,7 +96,6 @@ class BridgeTests: XCTestCase {
XCTAssertEqual(bridgeError, BridgeError.missingWebView)
}

@MainActor
func testEvaluateFunction() async throws {
let webView = TestWebView()
let bridge = Bridge(webView: webView)
Expand Down
1 change: 1 addition & 0 deletions Tests/ComponentTestExample/ComposerComponentTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import XCTest
import WebKit
import Strada

@MainActor
final class ComposerComponentTests: XCTestCase {
private var delegate: BridgeDelegateSpy!
private var destination: AppBridgeDestination!
Expand Down
5 changes: 5 additions & 0 deletions Tests/Extensions/TimeInterval+ExpectationTimeout.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Foundation

extension TimeInterval {
static let expectationTimeout: TimeInterval = 5
}
Loading
Loading