Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: async read function by default #368

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

drzraf
Copy link
Collaborator

@drzraf drzraf commented Oct 5, 2021

  • Remove the distinction between sync/async readFileFn in order to use a unified codepath.
  • Remove the need for calling the preprocessFinished() function after defining a (now async) preprocess() callback.

@drzraf
Copy link
Collaborator Author

drzraf commented Oct 5, 2021

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 #346 (comment))

@drzraf drzraf requested a review from AidasK October 5, 2021 17:01
@drzraf
Copy link
Collaborator Author

drzraf commented Oct 8, 2021

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

@drzraf
Copy link
Collaborator Author

drzraf commented Oct 12, 2021

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
Copy link

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
Copy link
Collaborator Author

drzraf commented Apr 5, 2023

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

 unified codepath.
- Remove the need for calling the preprocessFinished() function after defining
  a (now async) preprocess() callback.
@AidasK
Copy link
Member

AidasK commented Apr 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants