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

Support multipart form data #1880

Closed
wants to merge 9 commits into from
Closed

Support multipart form data #1880

wants to merge 9 commits into from

Conversation

goodexpert
Copy link

@goodexpert goodexpert commented Oct 19, 2018

See #1873

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@bajtos
Copy link
Member

bajtos commented Oct 19, 2018

Hi @goodexpert, thank you for the contribution, this is a great start! IIUC, you are building on top of #1838, thus we will need to land that pull request first, right?

Please sign our Contributor License Agreement, we cannot accept your contribution without that. See https://cla.strongloop.com/agreements/strongloop/loopback-next

Secondly, we will need you to add automated tests to cover your new feature. I would like to see at least one acceptance test showing how to write a controller method accepting file uploads.

Last but not least, the Travis CI build is failing in code-lint and commit-lint steps. You can fix most linting errors by running npm run lint:fix. To change the commit message, run git rebase -i master, select reword action for the commits needed changes and after the rebase is done, rewrite the history on GitHub by running git push --force-with-lease.

I highly recommend you to read our DEVELOPING guide to better understand our conventions.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

☝️

@goodexpert
Copy link
Author

I have fixed some of issue and added an unit testing of multipart/form-data

@bajtos
Copy link
Member

bajtos commented Oct 25, 2018

@goodexpert thank you for the updates. As I see it, adding support for file uploads is a big feature that will live with us for many months or years and will be difficult to significantly change. I'd like us to carefully design this figure in a way that will be robust, secure and extensible to support future needs. Let's discuss these high-level topics in #1873 first please, before looking at implementation details.

@kawasaki23Ven
Copy link

Is this available?

@goodexpert
Copy link
Author

Is this available?

Yes, I have applied this to my project to upload files with form data.

@goodexpert
Copy link
Author

Please check my new pull request.

@bajtos
Copy link
Member

bajtos commented Nov 8, 2018

Closing in favor of #1989

@bajtos bajtos closed this Nov 8, 2018
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