cmorten / opine

Minimalist web framework for Deno ported from ExpressJS.
https://github.com/cmorten/opine/blob/main/.github/API/api.md
MIT License
854 stars 43 forks source link

[#26] Proxied request body #86

Closed DxSimonovski closed 3 years ago

DxSimonovski commented 3 years ago

Issue

#26

Details

Added a converter method which returns a proxied version of the Request object to be used in middlewares so req.body resembles parsed value stored in req.parsedBody.

CheckList

github-actions[bot] commented 3 years ago

Benchmark results

PR to merge feat/issue-26-req-body-proxied c3830863618490754163493a34bd36f89fb33ab5 -> main ``` 1 middleware 5610.40 RPS 5 middleware 5605.59 RPS 10 middleware 5511.34 RPS 15 middleware 5406.67 RPS 20 middleware 5395.64 RPS 30 middleware 5398.09 RPS 50 middleware 4972.16 RPS 100 middleware 4052.77 RPS ```
DxSimonovski commented 3 years ago

@asos-craigmorten not very savvy around PRs so please correct me where needed or entirely disregard, I just want to help where possible :)

asos-craigmorten commented 3 years ago

Hey @DxSimonovski thanks for this! 🚀

I’m a little stacked atm so bare with me as might be slow to respond.

This looks great! CI is failing on lock files it looks like so, please can you run make lock (or copy and run the command in the Makefile if don’t have make on your machine) so we can get the test suite running as well.

One thing keen to have if this is to drop, is the ability to access the original body property via some raw so consumers don’t lose access to it. For backwards compatibility it would also be good to support and have a test asserting the parsedBody property still getting populated.

I’ll try and give the changes a proper review a bit later.

cmorten commented 3 years ago

@DxSimonovski apologies been slow getting around to looking at this!

Really excited to see this land - left a few comments and think need a merge down from main to get the CI green again.

cmorten commented 3 years ago

Superceded by https://github.com/asos-craigmorten/opine/pull/95