Skip to content

Conversation

@zach-klippenstein zach-klippenstein added this to the v1.0.0 milestone Jul 9, 2020
@zach-klippenstein zach-klippenstein requested a review from a team as a code owner July 9, 2020 20:49
import kotlin.test.fail

private const val WORKER_COUNT = 1000
private const val WORKER_COUNT = 500
Copy link
Collaborator Author

@zach-klippenstein zach-klippenstein Jul 9, 2020

Choose a reason for hiding this comment

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

Workers are a lot slower now, unfortunately. Fortunately, any optimizations we do for rendering Workflows will now also speed up workers, theoretically.

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/guwt-workers branch from 480469c to 4f6a074 Compare July 9, 2020 20:53
Comment on lines +205 to +218
inline fun <T, reified W : Worker<T>, PropsT, StateT, OutputT>
RenderContext<PropsT, StateT, OutputT>.runningWorker(
worker: W,
key: String = "",
noinline handler: (T) -> WorkflowAction<PropsT, StateT, OutputT>
) {
/* ktlint-enable parameter-list-wrapping */
runningWorker(worker, typeOf<W>(), key, handler)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't the API I'd ideally like to have, but since we don't have access to the full reflection library in this module, it's the best we can do. If we did, we could create the full worker type from the worker's class and the worker's outputType property, or maybe using the reified T type parameter. But since we don't, all we have is typeOf(), which means we need to reify the entire worker type.

However, while less flexible, it has the nice property that the type that is used to compare the worker will always be exactly the type for the worker that the IDE is showing you when you pass it to runningWorker. There's no mystery or hidden implementation details anymore. You might need to pass a few more keys in, but that's probably a good idea anyway.

* ```
*/
fun doesSameWorkAs(otherWorker: Worker<*>): Boolean = otherWorker::class == this::class
fun doesSameWorkAs(otherWorker: Worker<*>): Boolean = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unclear why these changes to true are okay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. The thinking was that the old default, comparing classes, would always be true now since the runtime itself is ensuring that worker types match. However, we ended up only comparing on the declared type of the worker, which means two workers that the runtime thinks are equivalent could still be different classes, so it would be safer to leave the class checks in here.

That said, by defaulting to relying on concrete types again, we lose the nice simplicity for testing. Not sure what the best thing to do is.

Copy link
Collaborator Author

@zach-klippenstein zach-klippenstein Jul 11, 2020

Choose a reason for hiding this comment

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

Reverted this default implementation, and added a bunch of tests around it. This implementation makes sense, because if the concrete type of a worker changes (e.g. was running a parent type, then runs a subtype with the same declared type), then the worker should restart. I need to document this somewhere…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Additional documentation added to runningWorker and doesSameWorkAs.

@zach-klippenstein
Copy link
Collaborator Author

Just found a pretty bad bug that blocks the release, but I want to fix separately: #120

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/guwt-workers branch 2 times, most recently from 6c2e14d to 76b2c37 Compare July 11, 2020 01:11
Issues:
 - Fixes #82, which is the Kotlin half of square/workflow#1021.
 - Fixes #92.
 - Fixes square/workflow#1197.
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.

Add a renderTester method for expecting typed workers Implement GUWT Workers Kotlin workers are started even if scope is cancelled

3 participants