flowjs / flow.js

A JavaScript library providing multiple simultaneous, stable, fault-tolerant and resumable/restartable file uploads via the HTML5 File API.
Other
2.96k stars 346 forks source link

WIP: async read function by default #368

Open drzraf opened 2 years ago

drzraf commented 2 years ago
drzraf commented 2 years ago

Tests are failing, this is related to the fact that once a queue is initialized, it relies upon recursion (so that xhr.load event handler calls the doneHandler which in turn enqueue the next chunk).

The problem with this is that while event handlers used to be synchronous... they now triggers async' function calls without returning the corresponding Promise to upper levels. In consequence, tests fail because they run assertions meanwhile the async XHR event handlers are still running in the background. (see https://github.com/flowjs/flow.js/issues/346#issuecomment-897658743)

drzraf commented 2 years ago

I'm pretty sure I'd have to rewrite uploadNextChunk and drop recursion altogether unless someone can provide an elegant alternative.

drzraf commented 2 years ago

Next problem is that XHR onload callback behavior is clearly not the best choice in a Promise-based world. I know of project Promisifying XHR, but the best actual candidate to remove recursion would be to switch to fetch (which probably offer many other benefits).

But the problem with fetch is that upload-progress is standardized but not implemented (https://developer.mozilla.org/en-US/docs/Web/API/Request) except on Chrome > 85 under an experimental flag (https://web.dev/fetch-upload-streaming/)

We definitely don't want to hold v3 until major browsers supports fetch upload Stream out of the box. But I think v3 uploads are not correct until and revamp of the recursion (regarding doneHandler) is done (@ilessiivi)

ilessiivi commented 2 years ago

I agree that there is room for improvement in uploadNextChunk and I'd welcome a rewrite if you have an idea for it.

I'm not sure though, if the synchronous upload tests will ever work with async readFileFn even if the entire function chain up to the web request (regardless if it's XHR or later on, Fetch) is Promise based. There are plenty of tests with await flow.upload() and the progress tests would be executed too late if the web requests were awaited for.

Perhaps the networking related tests should be rewritten with events (flow.on('file-progress') etc.)? That would reflect the code's real world usage better, too.

drzraf commented 1 year ago

Regarding fetch upload progress: https://developer.mozilla.org/en-US/docs/Web/API/BackgroundFetchRegistration/progress_event#browser_compatibility (still not in Safari nor Firefox)

AidasK commented 1 year ago

@drzraf looks good, if you want, you can merge this. But please update the docs accordingly