Deprecate existing WorkerBinder.bind methods and provide recommended bindTo methods with RibDispatchers.Default

[FranAguilera]

Description:

Since #553 we added support to specify on which CoroutineDispatcher are presidio Worker will be bound. RibDispatchers.Unconfined was added as a default parameter in order to keep existing behavior and not be a breaking change.

Still, binding by default on the current caller thread within interactor means that by default we are binding workers on main UI thread leading to potential jank ANR etc just for binding those workers.

This is specially problematic when the list of workers being bound keeps increasing (e.g. hypothetically if we have 100 workers and each bind takes at least 50 ms this will result on the UI thread being blocked for at least 5000ms)

Proposal is to deprecate existing methods and provide new methods that by default will bind on Ribdispatchers.DEFAULT (similarly to RibCoroutineWorker). Still if there is a need for a Worker to change the thread where it will be bound we can rely on coroutineContext property to be overriden as following (default value is EmptyCoroutineContext):

  class MyUiWorker:com.uber.rib.core.Worker{
    override val coroutineContext: CoroutineContext = RibDispatchers.Main
  }

Demo call:

worker.bindTo(interactor)

NOTE: For calling on existing java files -> WorkerBinderKt.bindTo(ribWorker, this));

Related issue(s):

#595

Verified via Unit tests

[tyvsmith]

This looks like new functionality added that isn't needed for these changes (unless I missed it's usage) where we support binding to a list.

1. Should this be a separate PR if we want a new puiblic API.
2. Rather in this diff or on the next, probably rename to bindToPresenters (plural)

[FranAguilera]

We've been providing these APIs (WorkerBinder.bind(interactor, listOfWorkers)/ WorkerBinder.bind(presenter, listOfWorkers) for a while.

Part of the goal of this PR is to deprecate existing WorkerBinder.bind methods given that the methods with 2 parameter call takes a default RibDispatchers.Unconfined which often times results in binding on main thread.

Was thinking that adding new methods (bindToInteractor, bindToPresenter) that are opinionated on using to RibDispatchers.Default to prevent further additions of WorkerBinder.bind on main thread (we can easily rely on static analysis to force everyone to use the new API when new WorkerBinder additions).

Also another important caveat, if there is still a need for a legacy worker to be bound on the main thread this can be achieved by overriding Worker's coroutineContext property (this will ignore the default Dispatchers specified on WorkerBinder call)

[psteiger]

I don't have any conceptual objections. Exposing new API as public top-level function instead of nested inside WorkerBinder object is nice. And deprecating the previous API instead of changing it keeps retro-compat.

With that said, maybe we should consider deprecating Worker in favor of RibCoroutineWorker. I'm not sure it's worth to keep evolving Worker API. But if it's being done as a smoother transition pathway, I'm ok with it.

[FranAguilera]

I don't have any conceptual objections. Exposing new API as public top-level function instead of nested inside WorkerBinder object is nice. And deprecating the previous API instead of changing it keeps retro-compat.

With that said, maybe we should consider deprecating Worker in favor of RibCoroutineWorker. I'm not sure it's worth to keep evolving Worker API. But if it's being done as a smoother transition pathway, I'm ok with it.

I completely agree with eventually fully deprecating this old worker. The only missing piece for RibCoroutineWorker is monitoring option for binding duration (but given that by default is bound on RibDispatchers.Default. there is no much of a concern). Also we can validate a few of this RibCoroutineWorkers in prod and if no issues found we can completely mark worker as deprecated

[FranAguilera]

I don't have any conceptual objections. Exposing new API as public top-level function instead of nested inside WorkerBinder object is nice. And deprecating the previous API instead of changing it keeps retro-compat.
With that said, maybe we should consider deprecating Worker in favor of RibCoroutineWorker. I'm not sure it's worth to keep evolving Worker API. But if it's being done as a smoother transition pathway, I'm ok with it.

I completely agree with eventually fully deprecating this old worker. The only missing piece for RibCoroutineWorker is monitoring option for binding duration (but given that by default is bound on RibDispatchers.Default. there is no much of a concern). Also we can validate a few of this RibCoroutineWorkers in prod and if no issues found we can completely mark worker as deprecated

Actually, after thinking about a bit more I don't think we should keep expanding this WorkerBinder API given is going to be deprecated soon in favor of RibCoroutineWorker.

I was thinking though to add a migration mechanism to change the WorkerBinderDispatcher to other than Unconfined. In that manner we can migrate internally all worker binding outside Ui thread in a safe manner

[FranAguilera]

Will be putting a new PR for migration API options