haxetink / tink_http

Cross platform HTTP abstraction
https://haxetink.github.io/tink_http
33 stars 19 forks source link

Client upload progress #57

Open kevinresol opened 7 years ago

kevinresol commented 7 years ago

Suggest changing the api to:

function request(request:OutgoingRequest): Signal<Progress<IncomingResponse>>

and enum Progress<T> { Done(data:<T>); Progress(fraction:Float); Failed(error:Error);}

kevinresol commented 7 years ago

or to make sure the response won't get lost, request could return:

{
  progress: Signal<Float>,
  repsonse: Future<IncomingResponse>,
}
kevinresol commented 7 years ago

Hm... should even split to uploadProgress and downloadProgress

kevinresol commented 7 years ago

and need a way to abort

back2dos commented 7 years ago

Progress is tricky, because in either direction content-length needn't be set. I guess this is roughly the best we can do:

interface HttpTransaction {
  var bytesWritten(get, never):Observable<Int>;
  var bytesRead(get, never):Observable<Int>;
  var response(get, never):Future<IncomingResponse>;
  function abort():Future<Bool>;
}

Aborting is going to be a PITA, I can tell you. A lot of fun stuff like ensuring the body of the OutgoingRequest is properly closed to avoid keeping files open and what not. But sure, it can be done :)

kevinresol commented 7 years ago

Can we still include content length and make it Optional?

kevinresol commented 5 years ago

Now we have tink.state.Progress, any new API proposal here?

kevinresol commented 3 years ago

I think it is just:

function request(req:OutgoingRequest):Progress<Outcome<IncomingResponse, Error>>;
back2dos commented 3 years ago

Hmm. Looking at the signature, I would probably expect to the progress to report on the download, rather than the upload.

I'm still inclined to think that we might want to return a more complex object that has an upload and download progress and the response. Or it could be something like:

function request(req:OutgoingRequest, ?handlers:{ ?upload:ProgressValue->Void, ?download:ProgressValue->Void }):Progress<Outcome<IncomingResponse, Error>>;

Still doesn't really handle the topic of cancelation though.

kevinresol commented 3 years ago

I wrongly thought that the response is only available after the upload is finished but it should not be the case. I think HTTP can produce a response header as soon as the request header is received, without the completely receiving the request body. So Progress<Outcome<IncomingResponse, Error>> is really no good.

As for download progress I thought it should be deduced from the reading of the response body Source, so I am not sure if we need/want a separate observer, unless the response body is given as a complete Chunk.

https://github.com/haxetink/tink_http/blob/f7ccf01f4b4e46eed9f448406a3d853a3fa8d72a/src/tink/http/Fetch.hx#L133-L176