-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allowing files to be deleted after upload #589
Conversation
Verrrry nice, thanks! I think this functionality is not specific to the XHRUpload plugin; i imagine you could also remove files that were uploaded using Tus or AwsS3. Maybe it should be in a separate |
src/core/Core.js
Outdated
@@ -411,6 +424,10 @@ class Uppy { | |||
const removedFile = updatedFiles[fileID] | |||
delete updatedFiles[fileID] | |||
|
|||
this.removers.forEach((remover) => { | |||
remover([fileID]) | |||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Processor and uploader functions are kept in arrays in core because you can have multiple. I think a single remover function should be enough; it doesn't need a pipeline like uploads may need.
Maybe this can be done by emitting a remove-file
event here in removeFile()
, and then listening for it in a DeleteFiles
plugin instead?
Yes, you right. It might be better. CoreI added two functions XHRUploadBack to original DeleteFilesA new plugin for deleting files uppy.use(DeleteFiles, {
endpoint: 'myawesomesite.com/delete',
method: 'delete'
}) |
Thanks for keeping this updated! I think rather than having an |
@@ -0,0 +1,84 @@ | |||
const Plugin = require('../core/Plugin') | |||
|
|||
module.exports = class DeletrFiles extends Plugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo in the name here and in the this.id
/ this.title
properties below :)
Hi there, I appreciate all the work being done in here, but I must admit that I have some concerns about this in its current state becoming part of 'official Uppy'. I want to share & name each of them so you can more easily weigh in: a) Delete isn't particularly standardized. Every framework has an abstraction for handling uploads through e.g. b) It's hard to guarantee a delete works even if the implementation tries hard. Looking at an ideal case of tus with delete implemented plus an encoding backend such as transloadit: you may succeed to delete an uploaded file, but there could already be resized / encoded versions of them out there on the internet. Yet the interface confusingly reports: c) If this addition is to pave the way to a Gallery functionality (an argument our team brought up - I have little experience with this but it might be interesting to explore), would it not be better to have this bundled in a GalleryPlugin? I assume such a Gallery would require more custom endpoints besides delete. So why not have all that in one place. d) How should it look in the UI? Currently, the Hope this makes sense, happy to hear your thoughts and maybe clear away some of these concerns! |
Hi there, I've written some remarks. {
endpoint: 'myawesomesite.com/delete',
method: 'post',
events: ['file-remove', 'file-remove-failed']
onFailEvent: 'file-remove-failed'
} b) You can listen 'file-remove-failed'. c) We can have an extra option for Dashboard e.g. 'allowRemoveAfterUpload' d) If we want to not get people confused green 'v' sign should be displayed after upload anyway. It might be moved to the left corner, or it might be moved to the left a bit. I hope to hear your opinion about this, after I can write code. P.S. Sorry for my English. It's not my native one. |
Hi there, I'm terribly sorry about the late reply! We've been having internal discussions about this, and also feeling guilty about the work you already put in.
It's certainly an interesting abstraction, but for it not to be a leaky one, it would require more. Like, if the action is configurable, the icon also needs to be configurable, and maybe even the placement. The request might need some kind of auth, or a dynamically generated URL. And perhaps, it actually needs to be a little context aware menu, if the aim is to support multiple actions per file such as indeed, remotely purge/rename/tag, or whichever extra actions would make for the gallery or app at hand.
In the example the delete worked correctly, but another process already picked up the file, and has converted this to 5 different sizes and published on the internet. Just judging by wether the delete of the original file worked, we would be lying to the user that the file is gone.
Sorry, I'm unsure how this relates to the GalleryPlugin proposal! But to talk just about what was proposed here, i'd think as a rule of thumb we should try to not have a Plugin's config leak into the Dashboard Plugin if we can avoid it :)
To me that doesn't change that the delete icon meaning changes from first being: "actually, don't change remote state", to: "change remote state" (and then not being able to guarantee if remote state did indeed change). To say I'm not proud about how long I've let this linger is an understatement. As it stands tho, I think there are too many question marks around this feature to ship it with core. And I fear these question marks would come back tenfold as support tickets. I feel it would be best to leverage Uppy's plugin friendly nature and publish your desired feature in a separate repo/module. We'd be happy to link to it in the community section, and I wouldn't be surprised if many people start using it and you get a significant following - but Uppy core is not the best place for this feature, at this time. I'm very sorry about the work that has been poured into this already, and the time it took us as core team to even come to some kind of an agreement (for which I solely am to blame!) Happy to further discuss, but I'll be closing this PR for now. |
Sometimes after files have been uploaded, the user wants to delete some of them before next step.
Core
Currently we are adding uploader like e.g.
The decision is provide second function e.g.
So each uploader can implement
upload
andremove
function.XHRUpload
For XHRUpload we can add a delete option like e.g.