Introduce runningSideEffect API #12
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
f97d8f4 to
942884f
Compare
942884f to
a5e19d2
Compare
b6a67a4 to
7bb3536
Compare
|
This PR is ready for review. |
workflow-runtime/src/main/java/com/squareup/workflow/internal/RealRenderContext.kt
Show resolved
Hide resolved
workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt
Outdated
Show resolved
Hide resolved
workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt
Outdated
Show resolved
Hide resolved
workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt
Show resolved
Hide resolved
workflow-runtime/src/main/java/com/squareup/workflow/internal/SideEffectNode.kt
Outdated
Show resolved
Hide resolved
b78ebf3 to
65587b5
Compare
zach-klippenstein
added a commit
that referenced
this pull request
Jun 19, 2020
…end functions. - `Sink.sendAndAwaitApplication()` - `Flow.collectToSink()` These helpers will be used to implement Workers using side effects (#12). GUWT workers are described in square/workflow#1021.
2 tasks
65587b5 to
a25ef3d
Compare
zach-klippenstein
added a commit
that referenced
this pull request
Jun 20, 2020
…end functions. - `Sink.sendAndAwaitApplication()` - `Flow.collectToSink()` These helpers will be used to implement Workers using side effects (#12). GUWT workers are described in square/workflow#1021.
workflow-core/src/main/java/com/squareup/workflow/RenderContext.kt
Outdated
Show resolved
Hide resolved
First part of Kotlin implementation of square/workflow#1021. Kotlin counterpart to square/workflow#1174. This implementation intentionally does not run the side effect coroutine on the `workerContext` `CoroutineContext` that is threaded through the runtime for testing infrastructure. Initially, workers ran in the same context as the workflow runtime. The behavior of running workers on a different dispatcher by default (`Unconfined`) was introduced in square/workflow#851 as an optimization to reduce the overhead for running workers that only perform wiring tasks with other async libraries. This was a theoretical optimization, since running on the `Unconfined` dispatcher inherently involves less dispatching work, but the overhead of dispatching wiring coroutines was never actually shown to be a problem. Additionally, because tests often need to be in full control of dispatchers, the ability to override the context used for workers was introduced in square/workflow#940, which introduced `workerContext`. I am dropping that complexity here because it adds a decent amount of complexity to worker/side effect machinery without any proven value. It is also complexity that needs to be documented, and is probably just more confusing than anything. The old behavior for workers is maintained for now to reduce the risk of this change, but side effects will always run in the workflow runtime's context. This is nice and simple and unsurprising and easy to reason about.
a25ef3d to
a5906f9
Compare
rjrjr
approved these changes
Jun 23, 2020
| @@ -63,7 +67,7 @@ internal class WorkflowNode<PropsT, StateT, OutputT : Any, RenderingT>( | |||
| private val idCounter: IdCounter? = null, | |||
| initialState: StateT? = null, | |||
| private val workerContext: CoroutineContext = EmptyCoroutineContext | |||
| ) : CoroutineScope, WorkerRunner<StateT, OutputT> { | |||
| ) : CoroutineScope, WorkerRunner<StateT, OutputT>, SideEffectRunner { | |||
There was a problem hiding this comment.
Out of scope (ahem) for this PR, but: I forgot that this class implements CoroutineScope directly. Isn't that discouraged these days?
Have we already had this conversation?
There was a problem hiding this comment.
You are very right. I already fixed it in one of the other 300 branches I put up this weekend but i forget which one 😅
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.
First part of Kotlin implementation of square/workflow#1021.
Kotlin counterpart to square/workflow#1174.
This implementation intentionally does not run the side effect coroutine
on the
workerContextCoroutineContextthat is threaded through theruntime for testing infrastructure. Initially, workers ran in the same
context as the workflow runtime. The behavior of running workers on a
different dispatcher by default (
Unconfined) was introduced insquare/workflow#851 as an optimization to reduce
the overhead for running workers that only perform wiring tasks with
other async libraries. This was a theoretical optimization, since running
on the
Unconfineddispatcher inherently involves less dispatching work,but the overhead of dispatching wiring coroutines was never actually shown
to be a problem. Additionally, because tests often need to be in full
control of dispatchers, the ability to override the context used for
workers was introduced in square/workflow#940,
which introduced
workerContext. I am dropping that complexity herebecause it adds a decent amount of complexity to worker/side effect
machinery without any proven value. It is also complexity that needs to
be documented, and is probably just more confusing than anything. The
old behavior for workers is maintained for now to reduce the risk of this
change, but side effects will always run in the workflow runtime's
context. This is nice and simple and unsurprising and easy to reason
about.
Checklist
UI Tests(will be covered by existing UI tests when workers are migrated)