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

loaded & progress are not correct when file is upload #1001

Closed
mkszepp opened this issue Oct 12, 2023 · 1 comment · Fixed by #1002
Closed

loaded & progress are not correct when file is upload #1001

mkszepp opened this issue Oct 12, 2023 · 1 comment · Fixed by #1002
Labels

Comments

@mkszepp
Copy link
Contributor

mkszepp commented Oct 12, 2023

Steps to reproduce

  1. Add two or more files into queue
  2. Start upload (the easiest way to reproduce is, to make the upload sequential and throttle network connection to 3G or slow)
  3. First file is uploading and progress bar + loaded is increasing
  4. After the first file is completed the progress & loaded is decreased

Reason

On loadend event the event.total and event.loaded are taken the Content-Length header from the response (see here).
In my case this bug occures because the uploaded file was 12000 bytes and the Content-Length was 300 bytes, because my i my response body there is a json like { successful: true }

I think, there is not correct to set the Content-Length as file.size, because this is only correct, when i return after the upload the uploaded image.

While debugging, i have found an older issue. In this issue there was tried so fix this problem, by adding a check for total = 0 bytes (#475),
This means, that the progress & loaded works only when i return with status 200 & no body.

Possible fix

  • The file.size should only be set in onloadstart event
  • In Event onprogress set always only file.loaded
  • In event onloadend (when file is uploaded) we should set file.loaded equal to file.size

Actual code parts:

request.onprogress = function (evt) {
if (!evt) return;
if (!evt.lengthComputable || evt.total === 0) return;
file.loaded = evt.loaded;
file.size = evt.total;
file.progress = (evt.loaded / evt.total) * 100;
};

this.request.onloadstart =
this.request.onprogress =
this.request.onloadend =
bind(this, function (evt) {
this.onprogress?.(evt);
});

@gilest
Copy link
Collaborator

gilest commented Oct 18, 2023

Hey @mkszepp thanks for the detailed rundown. What you've described makes sense and I've suspicious that these progress numbers are not correct myself

@gilest gilest added the bug label Oct 21, 2023
gilest added a commit that referenced this issue Oct 21, 2023
An attempt to prove and model #1001 with a failing regression test

Unfortunately to do so I've also had to modify some internals...

Mirage upload handler 🌴
Adjusted this route handler to more accurately model a real upload request.

It now fires onloadstart, onprogress and onloadend rather then onprogress only.

The total value of the onloadend ProgressEvent is intentionally set to an arbitrary value.

⚠️ I don't think this is a breaking change, and I'd be surprised if it affected users of the mirage handler as it's a specific implementation detail of something this addon handles for users. No guarantees, though.

HTTPRequest 🌐
Setup so that onloadstart and onloadend may be called from the Mirage handler.

Shouldn't affect addon users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants