safeAction, safeEventHandler
#1221
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
189207d to
a5d53ea
Compare
rjrjr
commented
Sep 24, 2024
workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatefulWorkflow.kt
Outdated
Show resolved
Hide resolved
zach-klippenstein
approved these changes
Sep 24, 2024
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
740efac to
6b001b0
Compare
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
ad8a4f9 to
1485fa0
Compare
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
3 times, most recently
from
4c4f30a to
04c0440
Compare
rjrjr
commented
Sep 24, 2024
| @@ -155,6 +155,7 @@ public final class com/squareup/workflow1/Snapshots { | |||
| public abstract class com/squareup/workflow1/StatefulWorkflow : com/squareup/workflow1/IdCacheable, com/squareup/workflow1/Workflow { | |||
| public fun <init> ()V | |||
| public final fun asStatefulWorkflow ()Lcom/squareup/workflow1/StatefulWorkflow; | |||
| public final fun defaultOnFailedCast (Ljava/lang/String;Lkotlin/reflect/KClass;Ljava/lang/Object;)V | |||
There was a problem hiding this comment.
Why is this the only change caught by apiDump?
afollestad
approved these changes
Sep 24, 2024
@afollestad I actually called it |
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
027d916 to
3f9f98a
Compare
|
@afollestad obsessively tweaked the kdoc samples to say |
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
bf049f8 to
49abd90
Compare
workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatefulWorkflow.kt
Show resolved
Hide resolved
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
14d5ea3 to
cc76acf
Compare
People are confused by the fact that a `WorkflowAction` can't assume that a sealed class / interface `StateT` is the same subtype that it was at render time when the action fires. And those that do understand it resent this boilerplate:
```kotlin
action {
(state as? SpecificState)?.let { currentState ->
// whatever
}
}
```
So we introduce `StatefulWorkflow.safeAction` and `StatefulWorkflow.RenderContext.safeEventHandler` as conveniences to do that cast for you.
rjrjr
force-pushed
the
ray/give-me-convenience-or-give-me-death
branch
from
cc76acf to
10f5ef7
Compare
rjrjr
commented
Sep 25, 2024
| @@ -130,6 +130,45 @@ public interface BaseRenderContext<out PropsT, StateT, in OutputT> { | |||
| * given [update] function, and immediately passes it to [actionSink]. Handy for | |||
| * attaching event handlers to renderings. | |||
| * | |||
| * It is important to understand that the [update] lambda you provide here | |||
rjrjr
commented
Sep 25, 2024
| * Like [eventHandler], but no-ops if [state][WorkflowAction.Updater.state] has | ||
| * changed to a different type than [CurrentStateT] by the time [update] fires. | ||
| * | ||
| * It is also important to understand that **even if [update] is called, there is |
There was a problem hiding this comment.
@jeffhashfield Also added a callout here to that doc.
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| * This is not an uncommon case. Consider accidental rapid taps on | ||
| * a button, where the first tap event moves the receiving [StatefulWorkflow] | ||
| * to a new state. There is no reason to expect that the later taps will not | ||
| * fire the (now stale) event handler a few more times. No promise can be |
There was a problem hiding this comment.
the (now stale) event handler
Is it confusing to use the term event handler here? The WorkflowAction created by safeAction is probably being used to handle an event, but it might be better to use a different term that isn't easily confused with the eventHandler method. How about the (now stale) [update] lambda? That change would also work well in the safeEventHandler documentation.
| @@ -130,6 +130,45 @@ public interface BaseRenderContext<out PropsT, StateT, in OutputT> { | |||
| * given [update] function, and immediately passes it to [actionSink]. Handy for | |||
| * attaching event handlers to renderings. | |||
| * | |||
| * It is important to understand that the [update] lambda you provide here | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
People are confused by the fact that a
WorkflowActioncan't assume that a sealed class / interfaceStateTis the same subtype that it was at render time when the action fires. And those that do understand it resent this boilerplate:action { (state as? SpecificState)?.let { currentState -> // whatever } }So we introduce
StatefulWorkflow.safeActionandStatefulWorkflow.RenderContext.safeEventHandleras conveniences to do that cast for you.