-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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: x-forwarded-port header is 'undefined' when no port in url #60484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good @yuvalotem - can you come up with a test for it perhaps? You may be able to modify an existing one too. I will also look into this and then we can land your changes. Thank you!
Failing test suitesCommit: ab9642c
Expand output● app-dir-hmr › filesystem changes › should update server components pages when env files is changed (edge)
Read more about building and testing Next.js in contributing.md.
Expand output● Image Loader Config with Edge Runtime › dev mode › should add "src" to img1 based on the loader config
● Image Loader Config with Edge Runtime › dev mode › should add "srcset" to img1 based on the loader config
● Image Loader Config with Edge Runtime › dev mode › should add "src" to img2 based on the loader prop
● Image Loader Config with Edge Runtime › dev mode › should add "srcset" to img2 based on the loader prop
Read more about building and testing Next.js in contributing.md.
Expand output● app dir - not-found - group route › with runtime = edge › should use the not-found page under group routes
Read more about building and testing Next.js in contributing.md.
Expand output● interception-route-prefetch-cache › runtime = edge › should render the correct interception when two distinct layouts share the same path structure
Read more about building and testing Next.js in contributing.md.
Expand output● app dir - metadata › basic › should support other basic tags (edge)
Read more about building and testing Next.js in contributing.md.
Expand output● develop - app-dir - edge errros hmr › should recover from build errors when server component error
● develop - app-dir - edge errros hmr › should recover from build errors when client component error
Read more about building and testing Next.js in contributing.md.
Expand output● app-render-error-log › should log the correct values on app-render error with edge runtime
Read more about building and testing Next.js in contributing.md.
Expand output● app dir - css › css support › chunks › should bundle css resources into chunks
Read more about building and testing Next.js in contributing.md.
Expand output● app dir - Metadata API on the Edge runtime › should render OpenGraph image meta tag correctly
Read more about building and testing Next.js in contributing.md.
Expand output● Concurrent mode in the experimental-edge runtime dev › › should not have the initial route announced
● Concurrent mode in the experimental-edge runtime dev › should not have invalid config warning
Read more about building and testing Next.js in contributing.md.
Expand output● app dir - not-found - basic › with runtime = edge › should use the not-found page for non-matching routes
● app dir - not-found - basic › with runtime = edge › should match dynamic route not-found boundary correctly
● app dir - not-found - basic › with runtime = edge › should escalate notFound to parent layout if no not-found boundary present in current layer
Read more about building and testing Next.js in contributing.md.
Expand output● streaming dev dev › should support streaming for fizz response
● streaming dev dev › should not stream to crawlers or google pagerender bot
● streaming dev dev › should render 500 error correctly
● streaming dev dev › should render fallback if error raised from suspense during streaming
Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
buildDuration | 13.6s | 13.7s | N/A |
buildDurationCached | 7.3s | 6.2s | N/A |
nodeModulesSize | 198 MB | 198 MB | |
nextStartRea..uration (ms) | 430ms | 429ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
2453-HASH.js gzip | 30.5 kB | 30.5 kB | N/A |
3304.HASH.js gzip | 181 B | 181 B | ✓ |
3f784ff6-HASH.js gzip | 53.7 kB | 53.7 kB | N/A |
8299-HASH.js gzip | 5.04 kB | 5.04 kB | N/A |
framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
main-app-HASH.js gzip | 242 B | 242 B | ✓ |
main-HASH.js gzip | 32.2 kB | 32.2 kB | N/A |
webpack-HASH.js gzip | 1.68 kB | 1.68 kB | N/A |
Overall change | 45.6 kB | 45.6 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
_app-HASH.js gzip | 196 B | 197 B | N/A |
_error-HASH.js gzip | 184 B | 184 B | ✓ |
amp-HASH.js gzip | 505 B | 505 B | ✓ |
css-HASH.js gzip | 324 B | 325 B | N/A |
dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | N/A |
edge-ssr-HASH.js gzip | 258 B | 258 B | ✓ |
head-HASH.js gzip | 352 B | 352 B | ✓ |
hooks-HASH.js gzip | 370 B | 371 B | N/A |
image-HASH.js gzip | 4.21 kB | 4.21 kB | ✓ |
index-HASH.js gzip | 259 B | 259 B | ✓ |
link-HASH.js gzip | 2.67 kB | 2.67 kB | N/A |
routerDirect..HASH.js gzip | 314 B | 312 B | N/A |
script-HASH.js gzip | 386 B | 386 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 6.57 kB | 6.57 kB | ✓ |
Client Build Manifests
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
_buildManifest.js gzip | 481 B | 484 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
index.html gzip | 528 B | 529 B | N/A |
link.html gzip | 539 B | 541 B | N/A |
withRouter.html gzip | 523 B | 523 B | ✓ |
Overall change | 523 B | 523 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
edge-ssr.js gzip | 95.1 kB | 95.1 kB | N/A |
page.js gzip | 3.06 kB | 3.06 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 626 B | 626 B | ✓ |
middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
middleware.js gzip | 25.5 kB | 25.5 kB | N/A |
edge-runtime..pack.js gzip | 839 B | 839 B | ✓ |
Overall change | 1.62 kB | 1.62 kB | ✓ |
Next Runtimes
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 171 kB | 171 kB | ✓ |
app-page-exp..prod.js gzip | 96.5 kB | 96.5 kB | ✓ |
app-page-tur..prod.js gzip | 98.3 kB | 98.3 kB | ✓ |
app-page-tur..prod.js gzip | 92.7 kB | 92.7 kB | ✓ |
app-page.run...dev.js gzip | 149 kB | 149 kB | ✓ |
app-page.run..prod.js gzip | 91.2 kB | 91.2 kB | ✓ |
app-route-ex...dev.js gzip | 21.3 kB | 21.3 kB | ✓ |
app-route-ex..prod.js gzip | 15 kB | 15 kB | ✓ |
app-route-tu..prod.js gzip | 15 kB | 15 kB | ✓ |
app-route-tu..prod.js gzip | 14.8 kB | 14.8 kB | ✓ |
app-route.ru...dev.js gzip | 21 kB | 21 kB | ✓ |
app-route.ru..prod.js gzip | 14.8 kB | 14.8 kB | ✓ |
pages-api-tu..prod.js gzip | 9.52 kB | 9.52 kB | ✓ |
pages-api.ru...dev.js gzip | 9.8 kB | 9.8 kB | ✓ |
pages-api.ru..prod.js gzip | 9.52 kB | 9.52 kB | ✓ |
pages-turbo...prod.js gzip | 22.3 kB | 22.3 kB | ✓ |
pages.runtim...dev.js gzip | 22.9 kB | 22.9 kB | ✓ |
pages.runtim..prod.js gzip | 22.3 kB | 22.3 kB | ✓ |
server.runti..prod.js gzip | 50.5 kB | 50.5 kB | N/A |
Overall change | 897 kB | 897 kB | ✓ |
build cache Overall increase ⚠️
vercel/next.js canary | yuvalotem/next.js patch-2 | Change | |
---|---|---|---|
0.pack gzip | 1.56 MB | 1.56 MB | |
index.pack gzip | 105 kB | 105 kB | N/A |
Overall change | 1.56 MB | 1.56 MB |
Diff details
Diff for middleware.js
Diff too large to display
Diff for edge-ssr.js
Diff too large to display
Diff for server.runtime.prod.js
Diff too large to display
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot determine a way to test this issue using a local Next.js server (even a custom one).
I've created vercel/vercel#11254 to test this e2e. I've asserted that the currently described behavior in #61133 is valid (see commented output vercel/vercel#11254 (comment)).
Once this is landed, we can verify the new test passes and then land it to prevent regressions.
### What? See this issue - #61133 following this change #57815 `x-forwarded-port` header value is 'undefined' if the URL has no port ### Why? x-forwarded-port 'undefined' makes other http-proxy throw 405 error for the invalid header value ### How? Give default 80 port if the URL has no port --------- Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
What?
See this issue - #61133
following this change #57815
x-forwarded-port
header value is 'undefined' if the URL has no portWhy?
x-forwarded-port 'undefined' makes other http-proxy throw 405 error for the invalid header value
How?
Give default 80 port if the URL has no port