search

FineUploader / fine-uploader

Multiple file upload plugin with image previews, drag and drop, progress bars. S3 and Azure support, image scaling, form support, chunking, resume, pause, and tons of other features.
https://fineuploader.com
MIT License
8.19k stars 1.87k forks source link

1 - Upload Event called multiple times on same file #1141

Open EZWrighter opened 10 years ago

EZWrighter commented 10 years ago

From the documentation for the upload event: "Called just before an item begins uploading to the server."

I would assume it should only be triggered once per upload based on that. However it is being triggered after a pause/resume. A single time is my preference as well, as I need to take page actions based on the initial start of the upload.

feltnerm commented 10 years ago

Called just before an item begins uploading to the server.

Right. An "upload" is defined as when the client sends file data over HTTP to the server. So the behavior is correct in that it should emit the upload event twice if you have stopped uploading (paused) and then started again (resumed).

The submitted event would be the one you are looking for if you are looking to do something just before the initial upload request.

EDIT: After thinking about this more, submitted would only be a good workaround if autoUpload was set to true.

feltnerm commented 10 years ago

The upload event is called in a few places, notably when the uploader attempts to retry an upload, resume an upload, make the initial upload request, and/or continue a paused upload -- which appears to be your use case.

The emitting of the upload event when continuing a paused upload is due to the upload handler having no real concept of a paused upload. To pause an upload, Fine Uploader will simply abort the XHR request, and to continue it will send the request again. In existing design of Fine Uploader this will emit another upload event which is why you are running into this issue.

I would assume it should only be triggered once per upload [...] I need to take page actions based on the initial start of the upload.

What exactly would you define as the initial start of the upload? And what is your use case -- there could be another way...

EZWrighter commented 10 years ago

Thanks for the responses, so there are a few questions for me here.

First, I would define an 'upload' event to be after the upload has been submitted and confirmed and the first bit of data is beginning transfer. I don't really see a use for this event if it can be triggered at such weird times as you define. What good is it if you don't understand why the event was triggered.

Second, my use is around loading in dynamic data and changing the UI after the upload has commenced. It's a one time process per upload, no matter how many times they pause/resume.

I have worked around this bug by tracking current uploads outside your tool and skipping the init phase if it is already uploading.

I would suggest you update documentation for clarity and rework this event to make it useful. I doubt anybody actually uses it the way it is now.

rnicholus commented 10 years ago

We will look into updating the documentation, but the appropriate time to call onUpload is strictly a matter of personal preferences. The event is consistently called whenever a file upload kicks off. If an attempt fails, onUpload is called when the upload starts up again when it is retried at some point in the future. onUpload is really very appropriate for a resume, since this generally happens when a previously submitted and failed/aborted file from another session is submitted in a subsequent session, and Fine Uploader picks up where it left off using the auto resume feature. The logic for firing onUpload on a retry can be (and is) applied to a continuation of a paused upload.

We'll discuss more internally and update.

rnicholus commented 10 years ago

...I could see passing an additional parameter to onUpload that provides more context, such as a flag or flags that indicate this is associated with a resume oe retry.

EZWrighter commented 10 years ago

Hmmm, all those cases seem like resumes...and you already have an event for that.

rnicholus commented 10 years ago

A retry is not a resume. Also, continuing a paused upload is only implemented as a resume. As far as the user is concerned, it's a "continue".

EZWrighter commented 10 years ago

Ahh, then I would agree, a flag giving context to the upload event would probably be the most flexible and resolve my use case.

rnicholus commented 10 years ago

The pause/continue feature itself is a higher-level abstraction that the lower-level upload handler code is (mostly) not aware of (which is nice). When a user "pauses" an upload, the higher-level code asks the lower-level upload handler to abort the upload request. When a continue is requested, the upload handler is simply asked to "upload" the file, and its internal logic ends up resuming the upload, since at least one chunk uploaded successfully and there is persisted data to represent its progress when it was interrupted.

In an attempt to keep each piece of Fine Uploader's internal code focused on a specific job, these sorts of leaky abstractions are bound to pop up from time to time. I agree, a flag does seem like a reasonable solution to all of this, without creating any breaking changes.

EZWrighter commented 10 years ago

Personally, I would have preferred the events to be abstracted out into logical ones: uploadStarted/uploadCompleted, pause/resume, failed/retry. Those are all things an application developer can use to take action on. Right now you have a lot of event meaning overload that is very internal to the workings of your tool and obtuse to application layer developers. I understand not wanting to have breaking changes, but hopefully providing food for future thought :-)

rnicholus commented 10 years ago

We'll just update the documentation here to better clarify when this event is called.