Skip to content

Conversation

@arendjr
Copy link

@arendjr arendjr commented Jan 3, 2017

I have an alternative solution to fix #259.

The advantage to this PR is that the user of the multer library will get a proper error object indicating the reason for the failure ("client aborted"). An advantageous side-effect of this PR is that it removes a dependency. I have also added a test to simulate the situation.

A downside of this PR is that it introduces breaking API changes (another error code that introduces behavioral changes) and I had to change the Storage API so that the Disk Storage can know which file to remove when an unfinished file is canceled.

If desired, I could add a check to see if the callback to _handleFile() still receives the second info parameter and if it does it could still extend the file object with it. That way we would need to bring back the xtend dependency, but it would make it even easier for any users to upgrade to the new version, as any existing custom Storage implementations should remain compatible.

@arendjr arendjr changed the title Fix cleaning up temporarym files on client abort. Fix cleaning up temporary files on client abort. Jan 6, 2017
@LinusU
Copy link
Member

LinusU commented Feb 18, 2017

So sorry for the delay on this, the problem with merging this as is, is as you say the breaking changes. If we could somehow mitigate that I would be happy to merge this.

I've been working on Multer 2.x and just added the aborted handling that you worked on here to that branch. It still leaks temporary files though which needs to be adressed, should be easy to fix though with te new structure in Multer 2.x. Track the status of this here: #399

Again, if we can make this without a breaking change I promise to merge and release asap. But cutting a new breaking change doesn't really make sense with the 6 alpha releases of multer 2.x that is already out there...

Before merging this we should also run the unit tests for multer-s3 and multer-ftp (and any others that I'm just not yet aware of 😄). I will be happy to assist with this.

Thank you for your contribution, it's very appreciated!

@arendjr
Copy link
Author

arendjr commented Feb 21, 2017

Good to hear you're working on Multer 2! If this problem will be fixed there I'm happy to just leave this open for now and then switch once when version 2 is out.

While the breaking changes I made can be mitigated, at least for me adding the extra error code is necessary, which in itself should be considered a small breaking change, I think. So then I agree it's best to not break more than necessary and just hold out till version 2 :)

Thanks for your work!

@LinusU
Copy link
Member

LinusU commented Feb 21, 2017

Sounds like a plan, I hope to release soon 👍

If you want to try it out you can install it with: npm install --save multer@next

Documentation is currently here: https://github.com/expressjs/multer/blob/explore-new-api/README.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

file is not removed if upload is canceled

3 participants