uppy icon indicating copy to clipboard operation
uppy copied to clipboard

Make arguments passed to events consistent

Open Murderlon opened this issue 1 year ago • 7 comments

Initial checklist

  • [X] I understand this is a feature request and questions should be posted in the Community Forum
  • [X] I searched issues and couldn’t find anything (or linked relevant results below)

Problem

Event names are sometimes confusing and seem inconsistent.

Solution

@uppy/core

Always send file(s) instead of ID(s)

Renaming we decided not to do.
  • Rename files-added to file-batch-added. Or at least a better name to differentiate with file-added better
  • Rename upload to upload-start
  • Rename progress to upload-progress (total progress)
  • Rename upload-progress to file-progress (individual progress)
  • Rename complete to upload-complete (success or error)
  • Rename upload-error to file-error (one file upload error)
  • Rename error to upload-error (entire upload error)
  • Rename cancel-all to upload-cancel

@uppy/transloadit

  • It's slighly weird that we something prefix with assembly (transloadit:assembly-created) and sometimes not (transloadit:upload). We should always or never prefix with assembly because we are always talking about the same thing.
  • Make sure assembly is always the first argument, now it's mixed
  • Always send file(s) instead of ID(s)
  • Unclear what the difference is between transloadit:result and transloadit:complete. Potentially refactor to only have *:complete
  • uppy.on('error') is extended with an assembly property when the upload fails. This is a bit confusing, I suggest making sure transloadit:complete also fires with it fails, with the assembly context, and keep error generic / consistent no matter the upload plugin.

Related

These should also be taken into account:

  • https://github.com/transloadit/uppy/issues/4045
  • https://github.com/transloadit/uppy/issues/3248
  • https://github.com/transloadit/uppy/issues/3815
  • https://github.com/transloadit/uppy/issues/4333

Alternatives

  • No breaking change but rather confusing names for newcomers.
  • Support both to not make it a breaking change. Deprecate the old in the docs.
    • EDIT: having both is not possible as there are certain events (like upload-error) which would be both deprecated and new at once.

Murderlon avatar Aug 02 '22 15:08 Murderlon

We have internal, non-documented:

  • plugin-added
  • plugin-removed

arturi avatar Oct 06 '23 14:10 arturi

Reusing the upload-progress and upload-error names for a different event is unfortunate as code that is not properly migrated will still register a listener for an existing event but for a different one.

stof avatar Nov 07 '23 10:11 stof

our file-related events are a bit messy. some include a UppyFile object, other include fileId, some don't include anything.

in a future major we should probably refactor all file related events to either

  • always include fileId
  • or always include an up to date UppyFile object (from uppy.getFile())

mifi avatar Feb 15 '24 14:02 mifi

@mifi this could be improved in a minor version by adding the missing property (either fileId or file depending on the choice done for the future) in existing events in addition to the current properties (and documenting that the other one is deprecated)

stof avatar Feb 15 '24 14:02 stof

yes, but it will still be messy because fileId, file etc are not properties, they are instead positional arguments, so some events would be (fileId, file), others (file), others (file, fileId)

mifi avatar Feb 15 '24 14:02 mifi

One way or another I think we would need a breaking change. But I'm starting to change my mind on the renaming of all events. It's incredibly challenging to get right for little user value. Yes the current names are confusing, but with good docs we get away with it and we would prevent breaking changes.

That being said we should definitely make the arguments consistent and up-to-date (no stale file's)

Murderlon avatar Feb 15 '24 15:02 Murderlon

@mifi for positional arguments, there is indeed no backward compatible solution.

stof avatar Feb 15 '24 15:02 stof