mbraak/django-file-form

Callbacks or event emitter

mbraak opened this issue · 8 comments

We now have two apis that handle callbacks: the callbacks and the events like addUpload.

Let's see if it is possible to use a single api.

  • Possible solution is to use the callbacks option for all events.
  • The goal is to not break existing code.
  • Possible step: see if the new events can be handled by the callbacks option.

Yes, it does appear to be some duplicated functionality here.

As far as I can tell, callbacks are only for Upload, so they do not yet work for S3, placeholder, and reload of form. Of course more callbacks can be added, at least to S3, but the advantage of signal is that the same signal could be emitted by different mechanisms ( e.g. addUpload by placeholder, S3, tus etc), whereas multiple callbacks would have to be defined for each of them. (I am likely wrong here).

On the other hand the event interface does not yet handle onProgress, onCancel, onError etc, which I assume would be easy to add.

So

Possible solution is to use the callbacks option for all events.

I agree, but the events mechanism appears to be more powerful, and more modern.

The goal is to not break existing code.

If so decided, we could deprecate callbacks by removing its documentation but keeping the implementation for backward compatibility.

Possible step: see if the new events can be handled by the callbacks option.

Not possible without expanding callbacks, as far as I can tell.

Edit: Another thought, callbacks will be called in the same process/thread in which an event happens so they must be short. Events are executed asynchronously and can perform tasks that are more elaborative. In this regard events are flexible and will make the frontend more responsive. I would prefer events to callbacks.

Note that documentation on initUploadFields misses option eventEmitter.

options (optional)
    callbacks: callbacks for things like upload progress and errors.

Thanks, I'll update the documentation.

I agree that we should use the event emitter mechanism and deprecate the callbacks option. There is one thing I like to change, though. I would like to remove the eventEmitter parameter, and do the following:

const fileForm = initUploadFields(...);
fileForm.on('addUpload', ...);

The only problem with this is that addUpload events for the initial uploads are not handled. Because the events are already emitted after initUploadFields is called.

The only problem with this is that addUpload events for the initial uploads are not handled. Because the events are already emitted after initUploadFields is called.

Then it will be difficult to implement our example, which has placeholders.

It must be possible to find a solution for the placeholders: perhaps an option onAddInitialUpload. Another solution is to keep the eventEmitter option, but also return a fileForm object that emits events.

There is no activity on this issue. Closing it for now.