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

Fix proxy bypass return false #1696

Merged
merged 8 commits into from
Mar 19, 2019

Conversation

mistic
Copy link
Contributor

@mistic mistic commented Feb 28, 2019

  • 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

Based on the explanation in the docs

It must return either false or a path that will be served instead of continuing to proxy the request.

It was expected that return false skip the proxy with a 404

Breaking Changes

None

Additional Info

Closes #1677

/CC @evilebottnawi

@jsmith-sapient @jshado1 @evilebottnawi could you please test this manually and confirm that this have the intended behaviour?

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #1696 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1696      +/-   ##
==========================================
+ Coverage   83.76%   83.85%   +0.09%     
==========================================
  Files           8        8              
  Lines         536      539       +3     
  Branches      161      162       +1     
==========================================
+ Hits          449      452       +3     
  Misses         70       70              
  Partials       17       17
Impacted Files Coverage Δ
lib/Server.js 79.05% <100%> (+0.16%) ⬆️

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 34a4a26...5f3fa33. Read the comment docs.

lib/Server.js Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, just question

lib/Server.js Outdated Show resolved Hide resolved
Copy link

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

The suggested change (multiple nested ternaries and ifs) is too convoluted and has no code comments to help explain. No-one looking at this 5 minutes later would understand it 😨

lib/Server.js Outdated Show resolved Hide resolved
@mistic
Copy link
Contributor Author

mistic commented Mar 7, 2019

@evilebottnawi @jshado1 thanks for the suggestions. I tried to make the logic more clear and simple.

Copy link

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌

lib/Server.js Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good job!

@mistic
Copy link
Contributor Author

mistic commented Mar 19, 2019

@evilebottnawi I think you can merge this 😄

@alexander-akait alexander-akait merged commit aa7de77 into webpack:master Mar 19, 2019
BH-M87 added a commit to BH-M87/generator-cool that referenced this pull request Apr 9, 2019
? proxyConfig.bypass(req, res, proxyConfig)
: null;

if (typeof bypassUrl === 'boolean') {

Choose a reason for hiding this comment

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

This is not aligning to the documentation

In the function you get access to the request, response and proxy options. It must return either false or a path that will be served instead of continuing to proxy the request.

Copy link
Member

Choose a reason for hiding this comment

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

PR welcome to webpack docs

Choose a reason for hiding this comment

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

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.

proxy.bypass return false does not work
6 participants