[Swift] Rename compose to render and update docs.
#301
Conversation
davidapgar
requested review from
bencochran,
zach-klippenstein,
timdonnelly and
rjrjr
|
💯 |
| /// screen model. | ||
| /// | ||
| /// - Parameter state: The current state. | ||
| /// - Parameter context: The workflow context is the composition point for the workflow tree. To use a nested | ||
| /// workflow, instantiate it based on the current state. The newly instantiated workflow is | ||
| /// then used to invoke `context.render(_ workflow:)`, which returns the child's `Rendering` | ||
| /// type after creating or updating the nested workflow. | ||
| func compose(state: State, context: WorkflowContext<Self>) -> Rendering | ||
| func render(state: State, context: WorkflowContext<Self>) -> Rendering |
There was a problem hiding this comment.
@zach-klippenstein @timdonnelly @bencochran With the name of render here now, does the name of WorkflowContext still make sense?
I am starting to think that RenderContext would be a better type name - as it evokes that this that is context is only valid during the call to render (which has been a usage issue, of capturing and trying to use the context after the render pass is complete).
Not necessary to do it in this change, but wanted to discuss if it makes sense. This came to mind while talking to @bencochran regarding the SideEffects PR, as we now have different contexts, none of which are the "workflow"'s context - more just a context scoped to a single method.
There was a problem hiding this comment.
I would be super happy to rename to RenderContext.
AFAIK we haven't had as much of a problem with people trying to capture it, but if we start to see that one way to discourage it in Kotlin is to make it a receiver on render. It's a somewhat common pattern for FooContext types to be receivers on foo functions in kotlin. We haven't done that yet because we prefer making it clear what you're calling methods on, but the stdlib authors obviously feel differently about this.
There was a problem hiding this comment.
It's only happened a few times, but mostly when someone is trying this out for the first time. The current behavior is "surprising" as a sink created after render is complete doesn't go anywhere, which leads to the behavior where events from a Screen don't get back to the workflow.
I have #269 as an issue to make it assert if an invalid context is used, to at least make it obvious what's happening. But the naming (I would hope) is an extra signal about the scope.
There was a problem hiding this comment.
Updated this PR to also handle the name update to RenderContext as well as the docs.
davidapgar
force-pushed
the
davidapgar/266-rename-compose-to-render
branch
from
7e7d4ca
to
913b98b
Compare
davidapgar
force-pushed
the
davidapgar/266-rename-compose-to-render
branch
from
913b98b
to
6d80065
Compare
Closes #266