Skip to content

[fix]: use weak reference to internal sinks when vending to clients #189

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

Merged
merged 2 commits into from
Feb 6, 2023
Merged

[fix]: use weak reference to internal sinks when vending to clients #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c50b79a
[fix]: use weak reference to internal sinks when vending to clients
jamieQ Feb 6, 2023
4bf098e
update acls and add tests
jamieQ Feb 6, 2023
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
13 changes: 6 additions & 7 deletions Workflow/Sources/SubtreeManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ extension WorkflowNode {
internal var onUpdate: ((Output) -> Void)?

/// Sinks from the outside world (i.e. UI)
private var eventPipes: [EventPipe] = []
internal private(set) var eventPipes: [EventPipe] = []

/// Reusable sinks from the previous render pass
private var previousSinks: [ObjectIdentifier: AnyReusableSink] = [:]
Expand Down Expand Up @@ -198,12 +198,11 @@ extension WorkflowNode.SubtreeManager {
func makeSink<Action>(of actionType: Action.Type) -> Sink<Action> where Action: WorkflowAction, WorkflowType == Action.WorkflowType {
let reusableSink = sinkStore.findOrCreate(actionType: Action.self)

let signpostRef = SignpostRef()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated change, but figured we might as well not instantiate this here since it isn't necessarily going to be used (we could also further improve this with some additional changes).

let sink = Sink<Action> { [weak reusableSink] action in
WorkflowLogger.logSinkEvent(ref: SignpostRef(), action: action)

let sink = Sink<Action> { action in
WorkflowLogger.logSinkEvent(ref: signpostRef, action: action)

reusableSink.handle(action: action)
// use a weak reference as we'd like control over the lifetime
reusableSink?.handle(action: action)
}

return sink
Expand Down Expand Up @@ -291,7 +290,7 @@ extension WorkflowNode.SubtreeManager {
// MARK: - EventPipe

extension WorkflowNode.SubtreeManager {
fileprivate final class EventPipe {
internal final class EventPipe {
var validationState: ValidationState
enum ValidationState {
case preparing
Expand Down
20 changes: 20 additions & 0 deletions Workflow/Tests/SubtreeManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,26 @@ final class SubtreeManagerTests: XCTestCase {
XCTAssertEqual(manager.sideEffectLifetimes.count, 1)
XCTAssertEqual(manager.sideEffectLifetimes.keys.first, "key-2")
}

func test_eventPipes_notRetainedByExternalSinks() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

confirmed this fails prior to the change introduced in this PR

weak var weakEventPipe: WorkflowNode<TestWorkflow>.SubtreeManager.EventPipe?
var externalSink: Sink<TestWorkflow.Event>?
autoreleasepool {
let manager = WorkflowNode<TestWorkflow>.SubtreeManager()

manager.render { context in
externalSink = context.makeSink(of: TestWorkflow.Event.self)
}

weakEventPipe = manager.eventPipes.last

XCTAssertEqual(manager.eventPipes.count, 1)
XCTAssertNotNil(weakEventPipe)
}

XCTAssertNotNil(externalSink)
XCTAssertNil(weakEventPipe)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍

}

private struct TestViewModel {
Expand Down