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

Also forward error requests to the proxy #2425

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

OliverJAsh
Copy link
Contributor

Fixes #2380

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

See #2380 and #2380 (comment).

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #2425 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2425      +/-   ##
==========================================
+ Coverage    93.8%   93.81%   +<.01%     
==========================================
  Files          34       34              
  Lines        1307     1309       +2     
  Branches      371      371              
==========================================
+ Hits         1226     1228       +2     
  Misses         79       79              
  Partials        2        2
Impacted Files Coverage Δ
lib/Server.js 96.73% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 638103f...29a822e. Read the comment docs.


this.app.use(handle);
// Also forward error requests to the proxy so it can handle them.
this.app.use((error, req, res, next) => handle(req, res, next));
Copy link
Member

Choose a reason for hiding this comment

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

It is create two instances of proxy that was reduce performance

Copy link
Contributor Author

@OliverJAsh OliverJAsh Feb 3, 2020

Choose a reason for hiding this comment

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

The proxy middleware is created once, outside of handle:

      proxyMiddleware = getProxyMiddleware(proxyConfig);

… and the result is used in handle on each request, be it a non-error or error request. So I don't think this change will hurt performance in any way.

If you really think this will hurt performance, are you able to provide an example to demonstrate that's the case?

Copy link
Member

Choose a reason for hiding this comment

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

Each request now run handle (

const handle = (req, res, next) => {
) twice so we potentially send require to middleware twice
return proxyMiddleware(req, res, next);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe that's true. handle will only run once:

  • If the request has not errored, handle will be called via app.use(handle).
  • If the request has errored, handle will be called via app.use((error, req, res, next) => handle(req, res, next)).

You can see this for yourself if you add some logging to handle.

Copy link
Member

Choose a reason for hiding this comment

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

@alexander-akait
Copy link
Member

Something broken on CI, I rerun tests

@alexander-akait
Copy link
Member

/cc @hiroppy @Loonride need review and I will do patch release

@alexander-akait
Copy link
Member

/cc @hiroppy @Loonride Feel free to leave feedback, I will make a patch release after merge

@alexander-akait alexander-akait merged commit e291cd4 into webpack:master Feb 5, 2020
@alexander-akait
Copy link
Member

Bit thanks, sometimes I get too much error flow, and do not have time to investigate them in detail, I apologize for the situation and am very glad that you yourself could find the problem and fix it

@OliverJAsh
Copy link
Contributor Author

@evilebottnawi No problem, thanks for reviewing my PR! 😄

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.

Some errors are not handled correctly for proxied requests
2 participants