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.

Sign up for GitHub

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

Jump to bottom

Wrap store.send in Task {} #46

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Wrap store.send in Task {} #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9f66fa
Make send async
bfollington Feb 23, 2024
8030c54
Lock task to MainActor
bfollington Feb 23, 2024
cff26f5
Change which part is deferred
bfollington Feb 29, 2024
4020901
Switch to DispatchQueue.main.async
bfollington Mar 1, 2024
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
16 changes: 11 additions & 5 deletions Sources/ObservableStore/ObservableStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,14 +352,20 @@ where Model: ModelProtocol
/// `send(_:)` is run *synchronously*. It is up to you to guarantee it is
/// run on main thread when SwiftUI is being used.
public func send(_ action: Model.Action) {
DispatchQueue.main.async { @MainActor in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we use this instead of the Task interface?

DispatchQueue is part of the old Grand Central Dispatch async API and does not coordinate with Swift's concurrency green thread implementation. Best avoided.

Copy link
Author

Choose a reason for hiding this comment

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

I'm having trouble getting the tests to consistently pass using Task { @MainActor in }, but on further experimentation this seems to be an issue regardless of approach.

Each test passes individually but running them together has them all competing to schedule tasks. Let's chat about the best approach on a call.

self.sendSync(action)
}
}

public func sendSync(_ action: Model.Action) {
if loggingEnabled {
let actionString = String(describing: action)
logger.debug("Action: \(actionString, privacy: .public)")
}

// Dispatch action before state change
_actions.send(action)

self._actions.send(action)
// Create next state update
let next = Model.update(
state: self.state,
Expand Down Expand Up @@ -388,10 +394,10 @@ where Model: ModelProtocol
self.state = next.state
}
}

// Run effects
self.subscribe(to: next.fx)

// Dispatch update after state change
self._updates.send(next)
}
Expand Down
36 changes: 20 additions & 16 deletions Tests/ObservableStoreTests/BindingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,16 @@ final class BindingTests: XCTestCase {
view.text = "Foo"
view.text = "Bar"

XCTAssertEqual(
store.state.text,
"Bar"
)
XCTAssertEqual(
store.state.edits,
2
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.text,
"Bar"
)
XCTAssertEqual(
store.state.edits,
2
)
}
}

/// Test creating binding for an address
Expand All @@ -86,13 +88,15 @@ final class BindingTests: XCTestCase {
view.text = "Foo"
view.text = "Bar"

XCTAssertEqual(
store.state.text,
"Bar"
)
XCTAssertEqual(
store.state.edits,
2
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.text,
"Bar"
)
XCTAssertEqual(
store.state.edits,
2
)
}
}
}
82 changes: 47 additions & 35 deletions Tests/ObservableStoreTests/ComponentMappingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,16 @@ class ComponentMappingTests: XCTestCase {
send(.setText("Foo"))
send(.setText("Bar"))

XCTAssertEqual(
store.state.child.text,
"Bar"
)
XCTAssertEqual(
store.state.edits,
2
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.child.text,
"Bar"
)
XCTAssertEqual(
store.state.edits,
2
)
}
}

func testKeyedCursorUpdate() throws {
Expand All @@ -164,11 +166,13 @@ class ComponentMappingTests: XCTestCase {
store.send(.keyedChild(action: .setText("BBB"), key: "a"))
store.send(.keyedChild(action: .setText("AAA"), key: "a"))

XCTAssertEqual(
store.state.keyedChildren["a"]?.text,
"AAA",
"KeyedCursor updates model at key"
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.keyedChildren["a"]?.text,
"AAA",
"KeyedCursor updates model at key"
)
}
}

func testCursorUpdateTransaction() throws {
Expand All @@ -194,15 +198,17 @@ class ComponentMappingTests: XCTestCase {
store.send(.setText("Woo"))
store.send(.setText("Woo"))

XCTAssertEqual(
store.state.child.text,
"Woo",
"Cursor updates child model"
)
XCTAssertEqual(
store.state.edits,
2
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.child.text,
"Woo",
"Cursor updates child model"
)
XCTAssertEqual(
store.state.edits,
2
)
}
}

func testKeyedCursorUpdateMissing() throws {
Expand All @@ -217,15 +223,18 @@ class ComponentMappingTests: XCTestCase {
environment: ()
)
store.send(.keyedChild(action: .setText("ZZZ"), key: "z"))
XCTAssertEqual(
store.state.keyedChildren.count,
3,
"KeyedCursor update does nothing if key is missing"
)
XCTAssertNil(
store.state.keyedChildren["z"],
"KeyedCursor update does nothing if key is missing"
)

DispatchQueue.main.async {
XCTAssertEqual(
store.state.keyedChildren.count,
3,
"KeyedCursor update does nothing if key is missing"
)
XCTAssertNil(
store.state.keyedChildren["z"],
"KeyedCursor update does nothing if key is missing"
)
}
}

func testKeyedCursorUpdateTransaction() throws {
Expand All @@ -244,9 +253,12 @@ class ComponentMappingTests: XCTestCase {
action: .setText("Foo"),
environment: ()
)
XCTAssertNotNil(
update.transaction,
"Transaction is preserved by cursor"
)

DispatchQueue.main.async {
XCTAssertNotNil(
update.transaction,
"Transaction is preserved by cursor"
)
}
}
}
14 changes: 8 additions & 6 deletions Tests/ObservableStoreTests/ObservableStoreTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ final class ObservableStoreTests: XCTestCase {

store.send(.increment)

XCTAssertEqual(store.state.count, 1, "state is advanced")
DispatchQueue.main.async {
XCTAssertEqual(store.state.count, 1, "state is advanced")
}
}

/// Tests that the immediately-completing empty Fx used as the default for
Expand All @@ -112,15 +114,15 @@ final class ObservableStoreTests: XCTestCase {
let expectation = XCTestExpectation(
description: "cancellable removed when publisher completes"
)
DispatchQueue.main.async {
DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
XCTAssertEqual(
store.cancellables.count,
0,
"cancellables removed when publisher completes"
)
expectation.fulfill()
}
wait(for: [expectation], timeout: 0.1)
wait(for: [expectation], timeout: 0.2)
}

/// Tests that immediately-completing Fx get removed from the cancellables.
Expand All @@ -145,15 +147,15 @@ final class ObservableStoreTests: XCTestCase {
let expectation = XCTestExpectation(
description: "cancellable removed when publisher completes"
)
DispatchQueue.main.async {
DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
XCTAssertEqual(
store.cancellables.count,
0,
"cancellables removed when publisher completes"
)
expectation.fulfill()
}
wait(for: [expectation], timeout: 0.1)
wait(for: [expectation], timeout: 0.2)
}

func testAsyncFxRemovedOnComplete() {
Expand Down Expand Up @@ -229,7 +231,7 @@ final class ObservableStoreTests: XCTestCase {
let expectation = XCTestExpectation(
description: "publisher does not fire when state does not change"
)
DispatchQueue.main.async {
DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
// Publisher should fire twice: once for initial state,
// once for state change.
XCTAssertEqual(
Expand Down
36 changes: 20 additions & 16 deletions Tests/ObservableStoreTests/ViewStoreTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,16 @@ final class ViewStoreTests: XCTestCase {

viewStore.send(.setText("Foo"))

XCTAssertEqual(
store.state.child.text,
"Foo"
)
XCTAssertEqual(
store.state.edits,
1
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.child.text,
"Foo"
)
XCTAssertEqual(
store.state.edits,
1
)
}
}

/// Test creating binding for an address
Expand All @@ -131,13 +133,15 @@ final class ViewStoreTests: XCTestCase {

viewStore.send(.setText("Foo"))

XCTAssertEqual(
store.state.child.text,
"Foo"
)
XCTAssertEqual(
store.state.edits,
1
)
DispatchQueue.main.async {
XCTAssertEqual(
store.state.child.text,
"Foo"
)
XCTAssertEqual(
store.state.edits,
1
)
}
}
}