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

Upgrade to 1.6.2-1 #1

Merged
merged 162 commits into from
Mar 4, 2021
Merged

Upgrade to 1.6.2-1 #1

merged 162 commits into from
Mar 4, 2021

Conversation

utay
Copy link

@utay utay commented Mar 3, 2021

Context in https://github.com/frontapp/front/pull/24480 and https://github.com/frontapp/front/pull/24496#discussion_r586373401.

TL;DR:
3-years-old issues addressed by our fork have been fixed (with other small bugs and way better error handling) on master so we could use express-http-proxy directly but there's a new issue (villadora#359) which makes GET requests to CloudFront/S3 fail because they have {} as the body. We can use this PR villadora#457 on the original repo to fix it (by not parsing the body if req.method === 'GET') but it hasn't been merged yet and looking at the maintainer activity, I think it won't be merged in the next weeks. So I ended up taking the latest master and applying this PR as a patch in our fork.

Steps done to upgrade + get patch from villadora#457:

  • git merge -X theirs villadora master
  • git merge v-electrolux master
  • Change name and version in package.json

LegNeato and others added 30 commits February 8, 2017 10:19
…cific, functional steps, and more formal consistency between the lifecyle hooks. (villadora#170)
* [refactor] Adding comments.

* [refactor] break sendProxy up slightly

* [refactor] straitening up sendProxyReq method
* [refactor] working on decorateRequestWrapper

* [refactor] incremental cleanup
…rify boundaries. Prep for standardizing workflow steps into thennables. (villadora#180)

REMOVES decorateRequest, ADDS decorateReqOpt and decorateReqBody.   This paves the way for pluggable bodyContent handlers, which has been a theme recently.
* extract resolveOptions to a module

* extract_decorateRequestWrapper
* extract buildProxyRequest, give parsing host its own workflow step

* Do not choose a random port number; causes test to fail when random number is already in use as a port number.
* Extract sendUserReq to module; other minor cleanup.

* Expanding comment.

* More comments.

* Further comments.   Improve property name.

* Use lint approved form of loose equality
* Remove errant debuggers; fix timeout test

* lint in subdirectories
* improve property name

* extract decorateProxyReqBody to a module
…ts sequential to avoid issues with passed-by-reference 'container'. (villadora#198)

Extract prepareProxyReq as a separate module
@utay utay self-assigned this Mar 3, 2021
@seanxiesx
Copy link

lgtm. would do a diff with upstream as a quick sanity check after you merge

@utay
Copy link
Author

utay commented Mar 3, 2021

lgtm. would do a diff with upstream as a quick sanity check after you merge

Good point. To make sure, I just ran in this repo:

for file in $(find . -type f -name '*.js' ); do
  diff $file path-to-upstream/express-http-proxy/$file
done
Diff output
39d38
<     var parseReqBody = (typeof options.parseReqBody === 'function') ? options.parseReqBody(req) : options.parseReqBody;
41c40
<     if (parseReqBody) {
---
>     if (options.parseReqBody) {
12,13c12
<   var parseReqBody = (typeof options.parseReqBody === 'function') ? options.parseReqBody(req) : options.parseReqBody;
<   var parseBody = (!parseReqBody) ? Promise.resolve(null) : requestOptions.bodyContent(req, res, options);
---
>   var parseBody = (!options.parseReqBody) ? Promise.resolve(null) : requestOptions.bodyContent(req, res, options);
67c67
<   describe('when user sets parseReqBody as bool', function () {
---
>   describe('when user sets parseReqBody', function () {
143,186d142
<   describe('when user sets parseReqBody as function', function () {
<     it('should not parse body with form-data content', function (done) {
<       var filename = os.tmpdir() + '/express-http-proxy-test-' + (new Date()).getTime() + '-png-transparent.png';
<       var app = express();
<       app.use(proxy('localhost:8109', {
<         parseReqBody: (proxyReq) => proxyReq.headers['content-type'].includes('application/json'),
<         proxyReqBodyDecorator: function (bodyContent) {
<           assert(!bodyContent, 'body content should not be parsed.');
<           return bodyContent;
<         }
<       }));
<
<       fs.writeFile(filename, pngData, function (err) {
<         if (err) { throw err; }
<         request(app)
<           .post('/post')
<           .attach('image', filename)
<           .end(function (err) {
<             fs.unlinkSync(filename);
<
<             done(err);
<           });
<       });
<     });
<     it('should parse body with json content', function (done) {
<       var app = express();
<       app.use(proxy('localhost:8109', {
<         parseReqBody: (proxyReq) => proxyReq.headers['content-type'].includes('application/json'),
<         proxyReqBodyDecorator: function (bodyContent) {
<           assert(bodyContent, 'body content should be parsed.');
<           return bodyContent;
<         }
<       }));
<
<       request(app)
<         .post('/post')
<         .send({ some: 'json' })
<         .end(function (err) {
<           done(err);
<         });
<     });
<   });
<
<
89d88
<   parseReqBody = (typeof parseReqBody === 'function') ? parseReqBody(req) : options.parseReqBody;

Looks good!

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.