search

veliovgroup / Meteor-Files

🚀 Upload files via DDP or HTTP to ☄️ Meteor server FS, AWS, GridFS, DropBox or Google Drive. Fast, secure and robust.
https://packosphere.com/ostrio/files
BSD 3-Clause "New" or "Revised" License
1.11k stars 167 forks source link

Update gridfs-streaming.md #819

Closed Prinzhorn closed 2 years ago

Prinzhorn commented 3 years ago

Correct me if I'm wrong, but this is not accurate. The files are streamed to the client using bucket.openDownloadStream, see https://github.com/veliovgroup/Meteor-Files/blob/master/docs/gridfs-bucket-integration.md#4-create-download-handler

That means the entire file does not need to be held in server memory. Correct?

jankapunkt commented 3 years ago

Why do you create a PR for each of these? I mean it's okay to have distinctive PRs but this is all about the gridfs documentation. I think this could all be handled in one PR having several commits for each changes. Makes reviewing also much easier.

cc @dr-dimitru

Prinzhorn commented 3 years ago

Because I'm following along the docs and open a PR when I run into an issue (using the GitHub UI). That is much easier on my end than to first keep a list of them and then go back to create a single PR and edit my fork and then push the changes. I though it would be a lot easier for you to hit merge five times than for me to keep updating a PR. Blame GitHub for having a limited "edit" button :smile:

Also I wasn't expecting to make multiple changes when I started and thought it would would remain a single PR.

jankapunkt commented 3 years ago

I though it would be a lot easier for you to hit merge five times than for me to keep updating a PR

That's not how a review works :-) So I have to make 5 reviews now and in the end one more final review to see if all the changed code examples work, when applying to a real app, for example like this one: https://github.com/veliovgroup/files-gridfs-autoform-example

jankapunkt commented 3 years ago

@Prinzhorn let me see if I can put them into one PR, git is fortunately not limited to what GitHub offers

Prinzhorn commented 3 years ago

Thanks!

Two of them are just text, nothing to test. One is literally syntax error (semicolon in an object literal). The two forgotten export make copy paste harder. And createAfterUpdate is a typo because the code that uses it has import { createOnAfterUpload } from 'path/to/createOnAfterUpload' and onAfterUpload: createOnAfterUpload(imageBucket),. They all currently prevent the code form working at all, so I'd be surprised if the example uses it.

I'm sorry for just throwing these PRs at you but I wanted to keep my flow and just spend the five seconds to open a PR via the UI.

I did get GridFS fully working now with these changes. (plus https://github.com/veliovgroup/Meteor-Files/issues/818)

jankapunkt commented 3 years ago

I will add code for gird-fs straming with GridFSBucket, since it is slightly different. However I first need to make this running in our project at work.