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

Refactor multiform upload to better implementation and handle uploading large file. this will fix #24, #25, #27 #31

Merged
merged 25 commits into from Nov 5, 2019

Conversation

onikiri2007
Copy link
Contributor

@onikiri2007 onikiri2007 commented Nov 4, 2019

  • Current multipart form upload implementation has the potential to run out of memory when uploading large files since it loads the whole file into memory and writes to the request file.
    this PR will attempt to fix that. code has been copied from Alamofire. their multiplatform data has a better implementation.

  • Refactor example app to send the direct video/image path instead of copying and let the platform take care of the rest.

This should fix with issue #24, #25, #27

@onikiri2007 onikiri2007 changed the title Refactor multiform upload to better implementation and handle large file upload. this will fix #24, #25, #27 Refactor multiform upload to better implementation and handle uploading large file. this will fix #24, #25, #27 Nov 4, 2019
@ened
Copy link
Collaborator

ened commented Nov 4, 2019

@onikiri2007 - Before going into much detail: Have you considered depending on Alamofire instead of copying the code in? We can always strip out unused components on a release build, therefore I don't think package size is a major concern at the moment.

Also, good to not copy a requested file twice (in the example), and let the form builder be standardised. Will the Alamofire uploader make a copy of the request and store it in cache, though?

@onikiri2007
Copy link
Contributor Author

@onikiri2007 - Before going into much detail: Have you considered depending on Alamofire instead of copying the code in? We can always strip out unused components on a release build, therefore I don't think package size is a major concern at the moment.
yeah that's a good idea. I'll change to make Alamorefire as dependency

Also, good to not copy a requested file twice (in the example), and let the form builder be standardised. Will the Alamofire uploader make a copy of the request and store it in cache, though? I'm not sure what you mean by this? Do you mean creating the request body as file? if this is the case, the answer is yes because urlsession only accept file as request body therefore It requires to create the one single file with all the files append. Alamofire's FormData class take care the better in this scenario.

@ened
Copy link
Collaborator

ened commented Nov 4, 2019

Also, good to not copy a requested file twice (in the example), and let the form builder be standardised. Will the Alamofire uploader make a copy of the request and store it in cache, though?

I'm not sure what you mean by this? Do you mean creating the request body as file? if this is the case, the answer is yes because urlsession only accept file as request body therefore It requires to create the one single file with all the files append. Alamofire's FormData class take care the better in this scenario.

OK, so it's only a code cleanup for the form data builder, sounds good!

@onikiri2007
Copy link
Contributor Author

first one is done. Alamofire is now dependency. I might refactor the Urlsession part also later to use Alamofire's sessionManager instead. for now this will be first refactoring.

@ened
Copy link
Collaborator

ened commented Nov 4, 2019

@onikiri2007 , ok - just two things left from my point of view:

  • please update the Podfile.lock (by running pod update)
  • please try to run SwiftLint or similar to remove the whitespace changes (a lot of lines' only change is addition of white spaces)

We'll do SwiftLint on CI in the future as well.

@onikiri2007
Copy link
Contributor Author

@onikiri2007 , ok - just two things left from my point of view:

  • please update the Podfile.lock (by running pod update)
  • please try to run SwiftLint or similar to remove the whitespace changes (a lot of lines' only change is addition of white spaces)

We'll do SwiftLint on CI in the future as well.

clean up is done
@ened Error: CocoaPods's specs repository is too out-of-date to satisfy dependencies. requires to update the podspec repo in CI

@ened ened mentioned this pull request Nov 5, 2019
@ened
Copy link
Collaborator

ened commented Nov 5, 2019

@onikiri2007 please rebase

@ened ened self-requested a review November 5, 2019 22:49
Copy link
Collaborator

@ened ened left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. As mentioned we will add the Swift linter next.

@onikiri2007 onikiri2007 added this to the Next Version milestone Nov 5, 2019
@onikiri2007 onikiri2007 merged commit 9064733 into fluttercommunity:master Nov 5, 2019
@onikiri2007 onikiri2007 deleted the fix2 branch November 5, 2019 23:00
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.

None yet

2 participants