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

Make arguments passed to events consistent #3938

Open
2 tasks done
Tracked by #4764
Murderlon opened this issue Aug 2, 2022 · 7 comments
Open
2 tasks done
Tracked by #4764

Make arguments passed to events consistent #3938

Murderlon opened this issue Aug 2, 2022 · 7 comments
Labels
4.0 For the 4.0 major version Feature

Comments

@Murderlon
Copy link
Member

Murderlon commented Aug 2, 2022

Initial checklist

  • I understand this is a feature request and questions should be posted in the Community Forum
  • 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:

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.
@arturi
Copy link
Contributor

arturi commented Oct 6, 2023

We have internal, non-documented:

  • plugin-added
  • plugin-removed

@arturi arturi mentioned this issue Oct 25, 2023
38 tasks
@stof
Copy link

stof commented Nov 7, 2023

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.

@mifi
Copy link
Contributor

mifi commented Feb 15, 2024

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())

@stof
Copy link

stof commented Feb 15, 2024

@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)

@mifi
Copy link
Contributor

mifi commented Feb 15, 2024

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)

@Murderlon
Copy link
Member Author

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)

@stof
Copy link

stof commented Feb 15, 2024

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

@Murderlon Murderlon changed the title Consistent, less confusing event names Make arguments passed to events consistent Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 For the 4.0 major version Feature
Projects
None yet
Development

No branches or pull requests

4 participants