RemoteData Applicative ordering

[quicksnap]

Currently, the applicative priority of RemoteData is initial > (pending, failure, success). Example:

// Outputs "Initial"
sequence(remoteData, array)([
  initial,
  pending,
  failure({}),
  success({}),
])

It makes more sense that any failure should result in failure. Then, any pending, then initial, and finally success.

Thoughts?

[raveclassic]

Thanks for the question.

Well I think that if we have a list of RemoteDatas and at least one of them hasn't been loaded yet (resolved) then we should treat the whole list as RemotePending because we can't say wether everything is RemoteSuccess ot RemoteFailure. The same applies for RemoteInitial (we don't have anything at all) so I'd say this is by design.

[quicksnap]

I suppose my reasoning is that, if we have a list of Successes and Failures, we would obviously show it as a failure. So, if we have a list of Successes, Failures and Pending, no matter what the outcome of that Pending, it will ultimately resolve to Failure anyway.

In practice, when we have a list of RemoteDatas that is meant to be represented as a single unit, once one fails, we want to display that to a user, since there's no way to recover without transitioning the Failure.

[joshburgess]

@raveclassic I think I agree with @quicksnap 's reasoning here, but it depends on what we want the fail-fast case to be, right?

When thinking about sequencing/traversing Options the fail-fast case is None. When thinking about sequencing/traversing Eithers the fail-fast case is Left. So, both of these treat the "left" or "failure" side of the branch as the fail-fast case.

It seems like your requirement for sequence/traverse returning a Failure is that all items are Failures, but is that really the desired outcome? I mean, yes, having all Failures should result in a Failure overall, but, really, if we are grouping RemoteDatas together & then using traverse/sequence, I think the expectation would be that even a single Failure being present would mean that we have to treat the overall operation as a Failure, right?

Like, if we have both a Pending and a Failure, we don't know yet whether or not the Pending item will succeed or fail, but we do know that something else has already failed. If we need both of those things to Succeed, this should be counted as an overall Failure, because it no longer matters what happens to the currently Pending item.... Or, at least, that's how I'm thinking about RemoteData right now... but I guess it's somewhat open to interpretation.

[joshburgess]

I thought it might be a good idea to check other implementations to see what they do. If we look at the apply (ap in fp-ts... which sequence & traverse rely on) implementation for RemoteData in PureScript, it seems that any combination including a Failure results in a failure overall. This is what I would expect.

https://github.com/krisajenkins/purescript-remotedata/blob/master/src/Network/RemoteData.purs#L58-L65

[quicksnap]

To recap the reasoning: once a failure has arrived, the outcome of that entire sequence is forever Failure so long as one exists, so any pending/initials are inconsequential to a sequence.

[raveclassic]

Yeah I see it makes sense. So what should be the correct priority? Failure > Pending > Initial > Success?

[raveclassic]

/cc @sutarmin

[sutarmin]

Makes sense for me too, nothing to add

[raveclassic]

@quicksnap So if you're ok with the priority would you like to open PR?

[gcanti]

@raveclassic IMO the current implementation is ok, its behaviour is consistent with the following:

RemoteData<L, A> is isomophic to Option<Option<Either<L, A>>>

[raveclassic]

@gcanti Well well.. I'm not so strong in maths but why should it be isomorphic?

[gcanti]

@raveclassic

RemoteData<L, A> = 1 + 1 + L + A
Option<Option<Either<L, A>>> = 1 + (1 + (L + A))

import { failure, initial, pending, RemoteData, success } from '@devexperts/remote-data-ts'
import { Either, left, right } from 'fp-ts/lib/Either'
import { none, Option, some } from 'fp-ts/lib/Option'
import { Iso } from 'monocle-ts'

const getIso = <L, A>(): Iso<RemoteData<L, A>, Option<Option<Either<L, A>>>> =>
  new Iso(
    s => s.fold(none, some(none), l => some(some(left<L, A>(l))), a => some(some(right<L, A>(a)))),
    a => a.fold(initial, oe => oe.fold(pending, e => e.fold<RemoteData<L, A>>(failure, success)))
  )

If you execute sequence on

const datas: Array<RemoteData<string, number>> = [initial, failure('foo'), success(1)]

what's the result? failure('foo')? However you must "wait for" data[0] to complete in order to get the eventual result

sequence([failure('bar'), failure('foo'), success(1)]) = failure('bar') // != failure('foo')

p.s.
This doesn't mean that you can't define other useful Applicative instances, just that the current one makes sense to me.

[raveclassic]

@quicksnap @joshburgess What do you think?

[joshburgess]

Hmmm, I guess that does make sense, but I think that, from a performance/usability perspective, failing-fast on any combination including a Failure seems like the more useful implementation. I mean, it seems like the functionality that we'd want most of the time. Why wait on currently in flight requests if we already have a Failure? Or... why fire off NotAsked, aka Initial, requests if we already have a failure? Then again, maybe we do still want to make those requests anyway. I guess it depends on what we intend to do after this point. Like, would there be any retry logic for Failures, etc.?

I'm not really sure what the answer is, but I'd think we'd want to treat the operation as a Failure as soon as we encounter one in the UI just so that we aren't waiting around on subsequent requests.

[sutarmin]

@joshburgess I thought absolutely the same way. But logic like canceling requests is not about RemoteData. And if we think about UI representation of your combined RDs, then in case of "fast-fail" applicative you will have UI-shown error that changes over time. If we use the new ordering:

sequence([pending, failure('foo')]) // failure('foo')
sequence(failure('bar'), failure('foo')]) // failure('bar')

The user has the first error, then somehow it changes to the other one. I find this behavior pretty weird.

[quicksnap]

@sutarmin in the use case of sequence, the UI would not usually show specifics for the error, since it would not get a collection of them all. So, failure('foo') vs failure('bar') likely makes no difference to what is displayed, since the user who called sequence should expect they will never receive every failure anyway.

[quicksnap]

@gcanti Does this isomorphism make sense for the other side of the discussion?

const getIso2 = <L, A>(): Iso<RemoteData<L, A>, Either<L, Option<Option<A>>>> =>
  new Iso(
    s => s.fold(right(some(none)), right(none), l => left(l), a => right(some(some(a)))),
    a => a.fold<RemoteData<L,A>>(failure, r => r.fold(pending, oe => oe.fold(initial, e => success(e))))
  )

[joshburgess]

@sutarmin I don't think we'd want to show specific failure messages for individual failures in this case (when using sequence/traverse to treat them as a group). I'd probably just add an interpretation step where I convert to a Left (Either) or a None (Option) and just show a general error message for the case of any of them being failures.

[quicksnap]

@raveclassic What should the action be on this? Do we want to change the existing behavior? I'd be happy to make the changes, but it would be a breaking change somewhat, since the behavior would be slightly different.

[raveclassic]

@quicksnap Yeah I'm afraid this's going to be a massive breaking change for us because all our projects rely on the fact that Pending has a higher priority.
Could we think about smth like adding several additional instanses of Apply for RemoteData which would not change the current behavior? If yes, what name could we pick for such different behavior?

[quicksnap]

@raveclassic from a consumer standpoint, how would opting in to those different Apply work?

[raveclassic]

We could export several instance constants (not only remoteData) with different ap implementation. Then we could pass those constants to sequence/traverse etc.

Also I think it's time to write some laws tests.

[joshburgess]

@raveclassic Maybe, we could have two separate versions under different directories? the new version could import and share code that would be the same from the current version. then, we could keep the exact same naming (still calling it remoteData), etc..... and people who want to use that version could just import from that directory and ignore the original?

Alternatively, the alternate version could be exported from the same file and just be called remoteData_ or something.

[joshburgess]

Any thoughts on the above? I could help create a PR as long as I know which approach you'd like to take.

[giogonzo]

Alternatively, the alternate version could be exported from the same file and just be called remoteData_ or something.

Something similar is already happening in fp-ts with Task and TaskEither, see e.g. https://github.com/gcanti/fp-ts/blob/master/src/Task.ts#L186-L194

[raveclassic]

I can't think of a proper name for such a "failing-fast" instance. Maybe remoteDataFast?

[joshburgess]

Maybe, something like remoteDataFailureBiased or remoteDataFailFast?

People who use it will probably use the as keyword to alias it as something shorter, like remoteData or remoteData_ anyway.

[joshburgess]

I created a PR to continue this discussion: #27

[raveclassic]

So we're fine with changing the priority. Feel free to open PR.

[joshburgess]

Ok, sounds good. I'll submit another PR soon.

[joshburgess]

I ended up tackling all of those things we mentioned in that closed PR all at the same time: changing the ap implementation, updating TypeScript, migrating from Traversable to Traversable2v (which also involved migrating to Foldable2v), and adding a Bifunctor instance.

I'm almost done. I just need to add tests for the new things now. I should have the PR up within the next day or two.

[joshburgess]

New PR open 👍 #28