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

Enhancement reassemble chunks #71

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

paulklemm
Copy link

In the node.js example, files are now assembled in the uploads folder after the upload is finished. Took me quite a while to figure this out how to do this today, so I would like to improve the example. This is discussed in Issue #17.

@AidasK
Copy link
Member

AidasK commented Jan 31, 2015

Looks ok, but will it work with simultaneousUploads > 1 ? Is status done returned only one time for files bigger than chunkSize?

@paulklemm
Copy link
Author

Is status done returned only one time for files bigger than chunkSize?

Yes, it is only called once. See here the output for a large file: https://gist.github.com/Powernap/bcca6e5a5ffc37cca630.

[...] will it work with simultaneousUploads > 1?

It should work, since the unique identifier is used for assembling the files.

@AidasK
Copy link
Member

AidasK commented Feb 1, 2015

Great, thanks!
How about updating file download logic too? https://github.com/Powernap/flow.js/blob/3e23096679f8e041024c3056088b5925f7afe115/samples/Node.js/app.js#L61
It should download file from uploads dir instead of merging chunks again.

And maybe we could clean all the chunks after the upload with https://github.com/Powernap/flow.js/blob/3e23096679f8e041024c3056088b5925f7afe115/samples/Node.js/flow-node.js#L180?

@paulklemm
Copy link
Author

And maybe we could clean all the chunks after the upload [...]
Done.

I'm with you, the download request should provide the assembled file! I could not find a way to get the filename from the identifier without writing a new method for it in the flow-node.js. What would be the proper way to do get the filename?

@AidasK
Copy link
Member

AidasK commented Feb 1, 2015

https://github.com/Powernap/flow.js/blob/3e23096679f8e041024c3056088b5925f7afe115/samples/Node.js/app.js#L23
Maybe we could save file by its ID and not by the file name?

@AidasK
Copy link
Member

AidasK commented Feb 1, 2015

For file download we could use modified route with given file name:

app.get('/download/:identifier/:name', function(req, res) {
   res.download('uploads/' +req.params.identifier, req.params.name);
});

Downloaded file will have its original name.
http://expressjs.com/api.html#res.download

@paulklemm
Copy link
Author

You are right, saving the file by ID makes much more sense. I will update the pull request.

@AidasK
Copy link
Member

AidasK commented Feb 17, 2015

How it is going with a pull request?

@paulklemm
Copy link
Author

Sorry, my Job and other responsibilities keep me busy, I did not forget it! I will push the request asap!

@AidasK
Copy link
Member

AidasK commented Feb 17, 2015

Glad to hear that you are on to it, but no need to hurry, pull request can wait, take your time. Thanks!

@weiway
Copy link

weiway commented Jul 16, 2015

How it going?

@paulklemm
Copy link
Author

So sorry for the delay. @AidasK Is this pull request still relevant? If so, I'll be happy to finish it asap.

@AidasK
Copy link
Member

AidasK commented Dec 10, 2015

Please do, thanks

@evilaliv3 evilaliv3 force-pushed the master branch 2 times, most recently from 360bba6 to b9e5421 Compare March 22, 2016 19:25
@aidenyang
Copy link

Any intention on merging this pull request? Just checking in since it looks like it's good to go; just hasn't been merged yet.

@paulklemm
Copy link
Author

Apparently the only change missing is saving the file by ID, other than that it should be ready. Sorry for taking so much time. Should be easy to do, but it is quite a while ago since I touched that code. I will look into it this week.

@paulklemm
Copy link
Author

Sorry for the delay. The files are now saved using their ID.

@AidasK The download routine does not work. It provides the proper path and file ID but the download fails. Do you have time to have a look at it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants