feat: remove bypass option from proxy configuration #5641
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5641 +/- ##
==========================================
- Coverage 83.62% 83.52% -0.10%
==========================================
Files 11 11
Lines 1960 1936 -24
Branches 727 717 -10
==========================================
- Hits 1639 1617 -22
+ Misses 288 286 -2
Partials 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return req.url; | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Can we rewrite these test to using new options from http-proxy-middleware
Also let's add in migration guide how to achive the same behaviour with new http proxy middleware
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
Let's improve couple things and we can merge
bjohansebas
force-pushed
the
remove-bypass-option
branch
from
120842a to
67c0784
Compare
|
Okay, I’ve removed those two tests because neither router nor pathRewrite allows sending responses. The ServerResponse object is not available there, unlike in bypass. In router, only the req object and the options passed as arguments can be accessed, and the same applies to pathRewrite. I’m working on adapting the other tests, because the behavior of bypass is completely different from what router and pathRewrite do. Hmm, I think it’s not possible to recreate those tests. The reason is that with bypass, the request never went through http-proxy-middleware, so in the first tests it was able to redirect to /index.html. With proxies, that route no longer exists, which means we would likely need to create a new route in the proxies. |
the router does not allow sending data the way bypass did. In the router we only have the req object, so this test is removed since it is invalid (https://github.com/chimurai/http-proxy-middleware/blob/6436ffc522a3a2485c198bd2fb544db0e33e3821/src/router.ts#L7)
Just like in the previous commit, the router, or any other handler ,does not allow sending data; it only changes where the request should be forwarded via the proxy. Therefore, it doesn’t make sense to keep this test.
bjohansebas
force-pushed
the
remove-bypass-option
branch
from
cf32348 to
d725363
Compare
test/server/proxy-option.test.js
Outdated
| }, | ||
| { | ||
| context: "/bypass-with-target", | ||
| pathFilter: "**/*.html", |
There was a problem hiding this comment.
I would expect that using router: "() => http://localhost:${port3}" should work and be somewhat similar to what bypass did, but it doesn’t fully work. I’m still thinking about a way to recreate bypass.
With bypass, the request never went through the proxy, which is why it could return index.html from the webpack-dev-server itself. Now we need to find a way for the request to go through the proxy and still reach the original server, without having to create a new route in the proxy.
| it("should wait if bypass returns promise", async () => { | ||
| const response = await req.get("/proxy/async"); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.text).toContain("proxy async response"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Neither router nor pathRewrite can send data directly since they don’t have access to the res object. So either it gets removed, or a new route is created in the proxy — but I’m not sure what the point of that would be.
| expect(response.text).toContain("from proxy2"); | ||
| }); | ||
|
|
||
| it("should allow req, res, and next", async () => { |
There was a problem hiding this comment.
Summary
This removes the option to bypass the proxy.
What kind of change does this PR introduce?
Did you add tests for your changes?
Does this PR introduce a breaking change?
If relevant, what needs to be documented once your changes are merged or what have you already documented?