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

Equatable sinks prototype #254

Draft
wants to merge 3 commits into
base: tomb/swiftui-testbed
Choose a base branch
from
Draft

Conversation

watt
Copy link
Collaborator

@watt watt commented Oct 30, 2023

No description provided.

}
}

// I guess this could be upstreamed to Blueprint
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I think I've seen this go by in POS

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just now noticing that SwiftUI's FocusState is not Equatable. That makes it harder for Views to be Equatable, but I presume SwiftUI's internal comparison of non-Equatable View types handles it well.

}
}

// I guess this could be upstreamed to Blueprint
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just now noticing that SwiftUI's FocusState is not Equatable. That makes it harder for Views to be Equatable, but I presume SwiftUI's internal comparison of non-Equatable View types handles it well.

Comment on lines -22 to +24
let close: (() -> Void)?

typealias Output = Never
enum Output {
case close
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could upstream deed364 to #240. Seems cleaner and more conventional than what I was doing there!

}

struct State {
var title: String
var isAllCaps: Bool
let trampoline = SinkTrampoline()
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like the best/only way to create a persistent identity that we can use to implement StableSink.== 👍

Comment on lines +97 to +101
let sink = context.makeSink(of: actionType)

sinks[ObjectIdentifier(actionType)] = sink

return StableSink(trampoline: self)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could pass sink into StableSink.init here and implement StableSink.send using that reference, allowing us to delete sinks, bounce, and destination. That would also avoid retaining any Workflow.Sink that is no longer referenced by any StableSink

Maintaining sinks does add some safety (against this, I think?) by ensuring we use the latest Workflow.Sink that the RenderContext has given us for a given Action type. But not total safety, because we're not evicting from sinks every sink that wasn't remade during the last render.

Is the need for that safety greater when the Rendering holds StableSinks rather than functions closing over Workflow.Sinks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants