Flatten error by removing ActionError

[dangthaison91]

This replaces PR for #138 and will resolve #119 (original by @bobgodwinx )

After referring Action's implementation from ReactiveSwift, I decide to add disabledErrors: Observable and remove ActionError. This will make breaking change but will help end users avoid misunderstand between .errors/.underlyingErrors and .executionErrors from the original proposal. I add ActionDisabledError for better debugging.

cc @ashfurrow @freak4pc

[bobgodwinx]

@dangthaison91 It looks nice 👍🏿 I would go this way if everyone is on the same page. Let's see what @fpillet @mosamer would add. Maybe we could add recoverySuggestion String? what do you guys think?.

[freak4pc]

A few nitpicks.
TBH I don't 100% understand the motivation for this PR. Could you clarify further what's the purpose of a ActionDisabledError specifically? Do you think removing the notEnabled vs underlyingError error is a good idea ? How do you differentiate between the two if you're actually interested in the specific error causing the action to fail ?

I'm wondering if this is too much of a breaking change but will leave the call to those more experienced with this lib than I am.

[freak4pc]

Can we align the markdown? I know its rendered right but as a person who likes to read raw markdown it bugs me :P

[dangthaison91]

Yeah, I will align them, just want to keep the PR is clean by keeping the original code.

[freak4pc]

What does this actually means? "due to being disabled", you meant?

[dangthaison91]

Your action will be disabled by enabledIf: Observable<Bool> and executing: Observable<Bool>. This is the default behavior of Action from the beginning.

[freak4pc]

Suggested change

	/// Delivered on whatever scheduler they were sent from.
	/// Delivered on the scheduler they originated from.

[dangthaison91]

I just want to keep original code/comment that I don't touch

[freak4pc]

If flattening is what you want, why not just expose a property ?

public enum ActionError: Error {
    case notEnabled
    case underlyingError(Error)
    
    var underlyingError: Error? {
        guard case .underlyingError(let error) = self else { return nil }
        return error
    }
}

or even

var error: Error {
    guard case .underlyingError(let error) = self else { return .notEnabled }
    return error
}

In my eyes, flattening should mean we still have access to both error types, not just "squish" them into one error and lose granularity.

[bobgodwinx]

@freak4pc using a struct is totally fine. I think the real Error should be left out of disabling and enabling. The code example you wrote was actually how I imagined it. But it tends to be confusing in the long run when Error emits and the work factory doesn't Complete. So pulling them apart is actually a good. Don't you think so?

[dangthaison91]

@freak4pc I use Action so much and all the time I only need to care about underlyingError. But sometimes, for new user disabledErrors will help them to debug and give them ideas about workFactory must completed to be enabled again.

This also makes align API with Action in ReactiveSwift.

[ashfurrow]

Hmm, I'm not really sure now. I initially thought this was a good idea (see my comment in #119) but I like @freak4pc's solution of the computed property. This would avoid a breaking change and avoid dividing errors into two properties (underlying errors and disabled errors). What do you think, @dangthaison91? Sorry for flip-flopping!

[dangthaison91]

@ashfurrow @freak4pc 's suggestion is exactly what we are doing right now but it is a little bit confusing for our newcomers when they use Action. They can be confused by .underlyingError and original errors. We just want to simplify the error API.

OTW, We can help people understand Action's behavior better by adding docs for handling errors.

[ashfurrow]

@dangthaison91 that's a good point. I think that @freak4pc's solution would be most complete with additional docs 👍

[freak4pc]

Agree IRT to docs, they're very lacking ATM in general (IMO) and could use some love. I really want to have some time to do this but TBH, it's probably better for someone else who uses it daily to write it :)

IRT to error flattening I still believe using a simple computed properly provides a singular "flattened" error without having to switch over anything, while still leaving the old behavior in tact and will act as a purely additive change - win win situation, in my book :)

[dangthaison91]

@freak4pc Should we ask people on Community channel how willing they are if we make a breaking change on Error API?

[freak4pc]

I think you'll need to get more users of Action who are actually convinced changing this is the right thing. It's not just about a breaking change, it's really about the fact I don't think this change provides so much value, TBH.

[bobgodwinx]

Hi @ALL This is the way I see it. enabled and disabled are things that we trigger when the work factory is executing. It's not an error. So I think real Errors should be treated as such in a different channel as a separate property. I might be wrong but it works for me in my app perfectly.

[freak4pc]

Observable<AationDisabledError> -> Observable<ActionDisabledError>

[khoi]

Suggested change

	- Added `disabledErrors: Observable<AationDisabledError>` for better debugging when workFactory not send `onCompleted()`
	- Added `disabledErrors: Observable<ActionDisabledError>` for better debugging when workFactory not send `onCompleted()`

[freak4pc]

That's actually an interesting point. OK, I sort of see what you're saying and I also re-read some of the code again.

I imagine the case for .notEnabled isn't one you often want to handle together with errors emitted from the workFactory - correct ?

In general, would you say a notEnabled error actually 100% of the time means a programmer has made an error? Or could it be a valid state?

I think my main points of this are :

1. If this isn't an Error, don't make it a Swift.Error to begin with.
2. If we're making breaking changes, let's think about the nicest way and not just a "middle" way.

I want to talk about this a bit more to make sure we're all aligned, if we're going down that path :)

[bobgodwinx]

@ALL I think @freak4pc got my point correct. We should find a better way to handle execution state For now I don't see a better way and if we treat it as error then the old code is just fine. Please pour in ideas.

[dangthaison91]

For now, we already had .isEnabled and .isExecuting as execution state. So, if we still want to continue with this PR, should we remove disabledError completely? We must add docs to help users aware of Action behavior with completed.

[bobgodwinx]

I am 50/50 for the documentation vs code. I believe in an API able to express it's self in what it's doing. The problem is that Action tends to throw the .notEnabled often and it's confusing. So this is why separating this into another channel might be good even though it's not the best idea. However let's think through all options.

[freak4pc]

Execution state is fine, but what you want to convey here is that "an input was received while the Action is disabled, you should check your WorkFactory stream to make sure it completed properly", correct?

In that case I'd say it is an error, and I'd be even more for having it as part of a simple computed property like I mentioned above - making it the developer's choice to handle that or not. They can simply ignore the actionDisabled error (or whatever you'd call it). This way we're still keeping backwards compatibility and it would make 100% sense for most use cases IMHO.

One more thing we can do here is adding a debug-only fatalError (much like we do in RxSwift-core), or even less aggressive - a debug-only runtime warning.

We can also possibly make the computed properties a bit more obvious / self-documenting:

var factoryError: Swift.Error? { // ... only underlying error } 
var isNotEnabled: Bool { // basic true/false without the error itself as it doesn't matter }

Thoughts?

[ashfurrow]

I would prefer @freak4pc's solution above if for no other reason than I don't want the API to encourage developers to ignore errors. In my opinion, calling execute() on an Action while it's already executing (and doesn't support concurrent execution) should produce an error. Users can ignore it if they want, but it shouldn't be separated into its own error property to make it easy to ignore, if that makes sense.

[freak4pc]

I think @ashfurrow's note makes sense. We shouldn't take steps to make this error easier to ignore as it might be hiding bigger issues.

[dangthaison91]

@freak4pc @ashfurrow You are right, we should produce an error while Action already executing.
My PR is just a cleaner API for convenience and may have some downside for other users, especially is new user. IMHO, who are using Action will be fine with both so we could do this if we get more question/issues from others.

I think adding fatalError like RxSwift won't help cause action can emit notEnabled very often. For example, I used Action to implement loadmore/pagination API while don't want to block user interaction.

[freak4pc]

I think adding fatalError like RxSwift won't help cause action can emit notEnabled very often. For example, I used Action to implement loadmore/pagination API while don't want to block user interaction.

Agree. I still think having a computed property + fixing the documentation would be the best path moving forward.

[freak4pc]

Are you still interested in pursing this PR @dangthaison91 ?

[freak4pc]

@dangthaison91 @bobgodwinx @fpillet Anyone interested in pursuing this approach? Otherwise I think it's worth closing as it's been open for a while with no progress.

[dangthaison91]

IMHO, user can easily create a computed property so not sure how can I help to improve this @freak4pc . It's fine to close this issue and revisit once we have more ideas?

[bobgodwinx]

I think this issue is still a good one. Let me see what I can do in the coming days. @dangthaison91 + @freak4pc