swift-composable-architecture icon indicating copy to clipboard operation
swift-composable-architecture copied to clipboard

Add a `EffectTask<Action>` typealias for `Effect<Action, Never>` and rename `Effect` to `EffectPublisher`

Open tgrapperon opened this issue 3 years ago • 7 comments

Specifying Never in Effect<Action, Never> is slightly inconvenient, and it happens the majority of the times Effect is used.

Without changing the shape of Effect, this PR introduces:

typealias EffectOf<Action> = Effect<Action, Never>

to remove the mental burden of Never and save a few keystrokes.

Now that TCA is slowly moving away from Combine, most dependencies are modeled using async/await, and Effect is becoming more and more focused on reducer's products. I think that a one-generic Effect<Action> was envisaged at some point (there are a few traces in the documentation that are fixed by this PR), but it would be a source-breaking change. If it ever happens someday, this typealias should allow codebases to have smoothly migrated toward this shape already, easing the transition.

An alternative would be to define EffectOf<R:ReducerProtocol> = Effect<R.Action, Never>. In both cases, they make sense, as one can see the reducer Reducer producing an EffectOf<Action> when it receives an Action, or an EffectOf<Reducer>.

My personal preference goes toward the solution proposed in this PR, which is mentally more lightweight, but the Reducer one also packs more information, so that could be useful at some point (although I don't see any real application right now).

What do you think about it?

tgrapperon avatar Oct 12 '22 05:10 tgrapperon

This sets in stone no evolution to add support of failure cases in reducers (which is the current direction anyhow 😕). Is TCA going to use the Effect type long term or opt for a strictly async return type once Combine is removed?

JacksonUtsch avatar Oct 12 '22 14:10 JacksonUtsch

This sets in stone no evolution to add support of failure cases in reducers (which is the current direction anyhow 😕). Is TCA going to use the Effect type long term or opt for a strictly async return type once Combine is removed?

Given our current modeling of the reducer, Effect<Action> is definitely the most natural direction to take things. This doesn't mean we're not open to exploring how reducer "failure" could work in general, e.g. what it would mean to add throws to the signature, or what it would mean to allow effects to throw. However, previous discussion has been pretty abstract, and neither Brandon nor I understand how it could be motivated in practice yet, especially without real world uses or demonstrations via modification of the existing case studies/demos.

Your discussion and implementation definitely sound interesting, but they lacked updated sample code and real examples that demonstrate what the changes bring to the table, and so it was difficult for me to truly comprehend.

One thing that has changed since your explorations is that the new Effect.task and Effect.run modifiers are throwing, giving you some flexibility especially around cancelling async work, but if they throw something other than cancellation it is up to you to handle things, or you will get runtime warnings.

stephencelis avatar Oct 12 '22 15:10 stephencelis

@stephencelis That is fair. It is quite an extensive change. It is a breaking change unless you can assume a generic type unless explicitly specified.

I have the strong preference of utilizing a result type rather than throws for type validation.

Not sure how to best demonstrate with sample code but will see if I can come up with something. 🙏

JacksonUtsch avatar Oct 12 '22 15:10 JacksonUtsch

@tgrapperon Thanks for this! @mbrandonw and I decided to flesh out our current vision for 1.0 here: https://github.com/pointfreeco/swift-composable-architecture/discussions/1477

This includes a plan for simplifying the Effect type. In the discussion we propose EffectTask<Action> as the migratory placeholder instead of EffectOf. Would you like to make that change here? If you'd like, you can also take a stab at the associated changes for that release, like the EffectPublisher rename. If not, no worries! We'll try to tackle after merging this :smile:

Let us know what you think! Or if you have any questions/concerns.

stephencelis avatar Oct 12 '22 19:10 stephencelis

(Oh, and if you do have things you wanna share, @JacksonUtsch, sorry to put more pressure on you to do so before 1.0, though if you don't have time to explore, there's always 2.0 😆)

stephencelis avatar Oct 12 '22 19:10 stephencelis

I renamed EffectOf to EffectTask, and renamed Effect to EffectPublisher according to #1477.

I had already updated all occurrences of Effect<_, Never> to EffectTask in the documentation.

I've gone one step further by updating naked Effect occurrences to EffectTask or EffectPublisher according to the context. Depending on the duration of the transition to 1.0, I feel that it is better to have a documentation that is coherent with what users are experiencing, especially onboarding ones. One can easily spot these occurrences by searching for '``EffectTask``' and '``EffectPublisher``' and replace by Effect if needed.

I made the choice of seeing Effect(value:) as an EffectTask given the fact that it is often (over)used as the return value of a Reducer, even if this value's implementation is pure Combine.

I didn't update one occurrence of Effect in its own documentation, as well as the ones from README. Any general reference like "effect" was also preserved.

I've replaced all internal uses of Effect to EffectPublisher, as if Effect was already hard-deprecated. When possible, EffectTask was used.

Effect is soft deprecated. It means that Xcode won't show a warning, nor a "fix" suggestion. I've opted to EffectPublisher as the new name for this value, as it's technically correct, but I feel that EffectTask would be more effective. In any case, as long as the deprecation is soft, this fix can't probably be activated, and the renamed: argument can probably be removed.

This is not intended to be a definitive version of the PR. I'm open to any change you would suggest!

tgrapperon avatar Oct 12 '22 22:10 tgrapperon

I'm good with the typealias , taking another direction for error handling. Don't believe any changes are needed for such.

JacksonUtsch avatar Oct 14 '22 21:10 JacksonUtsch

The reception to the change has been positive, so we're going to move forward with this. Thanks again @tgrapperon!

stephencelis avatar Oct 17 '22 22:10 stephencelis