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.

Sign up for GitHub

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

Jump to bottom

feat(rest): switch to body parser and add extensibility for parsers #1887

Closed
wants to merge 3 commits into from
Closed

feat(rest): switch to body parser and add extensibility for parsers #1887

wants to merge 3 commits into from

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Oct 19, 2018 •
edited
Loading

This PR adds the following features:

  1. switches to express body-parser:
  • body-parser is more actively maintained that body
  • body-parser supports compression options
  1. Allow body parsers to be plugged in
  • define an interface for BodyParser
  • allow body parser extensions to be bound to the context with REQUEST_BODY_PARSER_TAG.

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner October 19, 2018 22:23
@raymondfeng raymondfeng force-pushed the switch-to-body-parser branch from 480f8fc to c01602b Compare October 19, 2018 22:24
@raymondfeng raymondfeng force-pushed the support-form-body branch 2 times, most recently from d44f033 to 0764840 Compare October 22, 2018 17:43
@raymondfeng raymondfeng force-pushed the switch-to-body-parser branch from c01602b to fe1a354 Compare October 22, 2018 19:13
@raymondfeng raymondfeng changed the title Switch to body parser feat(rest): switch to body parser and add extensibility for parsers Oct 22, 2018
@bajtos
Copy link
Member

bajtos commented Oct 25, 2018

  • body-parser is more actively maintained that body
  • body-parser supports compression options

Those are good arguments for changing the underlying implementation 👍

I think your other pull request adding support for url-encoded request bodies (#1838) is very close to get landed. I expect severe merge conflicts to happen afterwards. Let's put this pull request on hold until #1838 is landed and you can update the patch to incorporate url-encoded support.

@raymondfeng raymondfeng force-pushed the support-form-body branch 5 times, most recently from 0c0d7d3 to a478e56 Compare October 26, 2018 14:44
@raymondfeng raymondfeng force-pushed the switch-to-body-parser branch 2 times, most recently from 3a107b5 to 4e1f204 Compare October 26, 2018 16:49
@raymondfeng raymondfeng force-pushed the support-form-body branch 2 times, most recently from ba40a29 to 61834d8 Compare October 29, 2018 15:05
@raymondfeng
feat(rest): add support for form request body
2d9e0a8 
See #1797

- allow applications to configure body parser options
- match content type to request body spec
- use RequestBody for validation
- extract unsupported media type error
@raymondfeng
feat(rest): switch to express body-parser
5076aea
@raymondfeng
feat(rest): allow body parsers to be plugged in
7390460
@raymondfeng raymondfeng force-pushed the switch-to-body-parser branch from 4e1f204 to 7390460 Compare October 29, 2018 15:58
@raymondfeng
Copy link
Contributor Author

github seems to have messed up this PR. Let me close it and open a new one.

@raymondfeng raymondfeng mentioned this pull request Oct 29, 2018
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos Awaiting requested review from bajtos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raymondfeng @bajtos