Add progress to RemotePending

[OliverJAsh]

I want to record the progress of an (upload) request, however currently RemotePending does not store any value for itself.

Example of a remote data type which has this: https://github.com/cyclejs-community/cycle-remote-data

https://github.com/cyclejs-community/cycle-remote-data#whencases-casest-u-u

Related krisajenkins/remotedata#25

[raveclassic]

@OliverJAsh This looks more like success state:

type Loading = { type: 'progress', progress: number }
type Resolved<A> = { type: 'resolved', value: A }
type Data<A> = Loading | Resolved<A>
type RD<A> = RemoteData<Data<A>>

Conceptually RemotePending describes initial loading of some piece of data and tracking progress IMO relates more to resolved (RemoteSuccess) state.

[OliverJAsh]

Conceptually RemotePending describes initial loading of some piece of data

That initial loading will have a progress though?

For context, I want to use this to track the progress of a file upload. The request is pending whilst the file uploads—it could still fail.

[raveclassic]

@OliverJAsh Makes sense... Still it will be a massive breaking change for our codebase to add an argument to pending. What do you think about adding optional argument to RemotePending constructor defaulting to zero and introducing new helper progress(value: number): RemotePending?

class RemotePending<L, A> {
  constructor(readonly progress: number = 0) { }
}
const progress = <L, A>(progress: number): RemotePending<L, A> => new RemotePending(progress)

Also we should think about Apply instance (and others). How such pendings with progress will be combined? Normalizing all progresses to 1? How should sequence/traverse behave for lists of RemoteData?

[OliverJAsh]

What do you think about adding optional argument to RemotePending constructor defaulting to zero and introducing new helper progress(value: number): RemotePending?

Good idea.

Do we need to make it explicit that the progress is for uploading (as it's typically derived from xhr.upload.onprogress, which emits ProgressEvent)? Perhaps:

type RemotePendingUpload = { progress: { loaded: number } };

class RemotePending<L, A> {
  constructor(readonly upload: Option<RemotePendingUpload> = Option.none()) { }
}

[raveclassic]

@OliverJAsh Progress can also be tracked for downloading, not only uploading, so I'd rather stick to progress: number argument. Is there anything else that should be tracked except this number?

[OliverJAsh]

True, in that case, what about a request where we want to track the progress of both uploading and downloading? We need two pieces of state I think?

Maybe we can also include the other properties of ProgressEvent: https://developer.mozilla.org/en-US/docs/Web/API/ProgressEvent

[raveclassic]

what about a request where we want to track the progress of both uploading and downloading?

Could you provide an example of such request?

Maybe we can also include the other properties of ProgressEvent

This doesn't play well with RemotePending default value. Option looks more like a crutch.

[OliverJAsh]

Option looks more like a crutch.

Sorry but can you elaborate?

Could you provide an example of such request?

I can't, sorry! Maybe it could be confusing if there was one progress state, that could be either the upload or download, depending on the implementation.

[raveclassic]

Sorry but can you elaborate?

I mean that if you already are in a pending state then progress === none makes no sense.

[raveclassic]

@OliverJAsh What do you think about making total an Option if lengthComputable is false?

type RemoteProgress = {
  loaded: number;
  total: Option<number>;
}
class RemotePending<L, A> {
  constructor(readonly progress: RemoteProgress= { loaded: 0, total: none }) { }
}
const progress = <L, A>(progress: RemoteProgress): RemotePending<L, A> => new RemotePending(progress)

[OliverJAsh]

@raveclassic Sounds sensible to me.

I still wonder if we need to allow for upload/download progress simultaneously, or at least to help disambiguate whether the progress signals an upload or download. 🤔

I mean that if you already are in a pending state then progress === none makes no sense.

Hmm, I see your point. OTOH, I don't think all requests emit a ProgressEvent (although I might be wrong), in which case I wouldn't expect a value.

[raveclassic]

I still wonder if we need to allow for upload/download progress simultaneously, or at least to help disambiguate whether the progress signals an upload or download.

I think it's not responsibility of RemoteData but of the process itself.

OTOH, I don't think all requests emit a ProgressEvent (although I might be wrong), in which case I wouldn't expect a value.

Agree, you are right since RemoteData is not only about XHR.

Summing up, is the following correct?

type RemoteProgress =  {
  loaded: number;
  total: Option<number>
}
class RemotePending<L, A> {
  contructor(readonly progress: Option<RemoteProgress> = none) { }
}
const progress = <L, A>(progress: RemoteProgress): RemotePending<L, A> => new RemotePending(progress)

Would you like to open PR?
There's one more important thing - what about combining RemotePendings? How should RemoteProgress be combined? Maybe normalized?

[raveclassic]

Here're my observations, need help with ???:

none + some({ loaded: 123, total: none }) = some({ loaded: 123, total: none })
{ loaded: 1, total: none } + { loaded: 2, total: none } = { loaded: 3, total: none }
{ loaded: 1, total: some(10) } + { loaded: 2, total: none } = ???

and the most tricky

const a = { loaded: 1, total: 10 }
const b = { loaded: 3, total: 20 }
const resultTotal = a.total + b.total // I guess the values should be added here, right?
const resultLoaded 
  = (a.loaded / (resultTotal / a.total)) / resultTotal + (b.loaded / (resultTotal / b.total)) / resultTotal 
  = (a.loaded * a.total + b.loaded * b.total) / (resultTotal * resultTotal)
  // correct?

/cc @sutarmin @igoralekseev @mankdev

EDIT: removed unnecessary Options for clarity

[sutarmin]

I think summing up proportionally would be the best strategy here.
??? looks like { loaded: 3, total: none }. Not enough data to assume smth better.

[raveclassic]

@sutarmin Makes sense
@OliverJAsh Is it acceptable for you?

[OliverJAsh]

I'm not sure to be honest. I haven't required that ability yet.

[raveclassic]

@OliverJAsh Please take a look #13