-
Notifications
You must be signed in to change notification settings - Fork 168
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
feat: direct ws connection fallback #4474
Changes from 3 commits
beeb019
c4ba769
96a4214
e734814
3a18f55
c9b2625
7bb52e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
import { promises as dns } from 'node:dns'; | ||
import { DevConfig, ServerConfig } from 'src/types/config'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to use relative path |
||
|
||
type Hostname = { | ||
host?: string; | ||
name: string; | ||
}; | ||
|
||
const wildcardHosts = new Set([ | ||
'0.0.0.0', | ||
'::', | ||
'0000:0000:0000:0000:0000:0000:0000:0000', | ||
]); | ||
|
||
export async function resolveHostname( | ||
optionsHost: string | boolean | undefined, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rsbuild's |
||
): Promise<Hostname> { | ||
let host: string | undefined; | ||
if (optionsHost === undefined || optionsHost === false) { | ||
// Use a secure default | ||
host = 'localhost'; | ||
} else if (optionsHost === true) { | ||
// If passed --host in the CLI without arguments | ||
host = undefined; // undefined typically means 0.0.0.0 or :: (listen on all IPs) | ||
} else { | ||
host = optionsHost; | ||
} | ||
|
||
// Set host name to localhost when possible | ||
let name = host === undefined || wildcardHosts.has(host) ? 'localhost' : host; | ||
|
||
if (host === 'localhost') { | ||
// See #8647 for more details. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to update this comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets remove it |
||
const localhostAddr = await getLocalhostAddressIfDiffersFromDNS(); | ||
if (localhostAddr) { | ||
name = localhostAddr; | ||
} | ||
} | ||
|
||
return { host, name }; | ||
} | ||
|
||
/** | ||
* Returns resolved localhost address when `dns.lookup` result differs from DNS | ||
* | ||
* `dns.lookup` result is same when defaultResultOrder is `verbatim`. | ||
* Even if defaultResultOrder is `ipv4first`, `dns.lookup` result maybe same. | ||
* For example, when IPv6 is not supported on that machine/network. | ||
*/ | ||
export async function getLocalhostAddressIfDiffersFromDNS(): Promise< | ||
string | undefined | ||
> { | ||
const [nodeResult, dnsResult] = await Promise.all([ | ||
dns.lookup('localhost'), | ||
dns.lookup('localhost', { verbatim: true }), | ||
]); | ||
const isSame = | ||
nodeResult.family === dnsResult.family && | ||
nodeResult.address === dnsResult.address; | ||
return isSame ? undefined : nodeResult.address; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code is copied from Vite up to line 61 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice if we could do some refactoring based on Rsbuild's needs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any special needs you would like? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the current version is LGTM 👍 |
||
|
||
export async function getResolvedClientConfig( | ||
clientConfig: DevConfig['client'], | ||
serverConfig: ServerConfig, | ||
): Promise<DevConfig['client']> { | ||
const resolvedServerHostname = (await resolveHostname(serverConfig.host)) | ||
.name; | ||
const resolvedServerPort = serverConfig.port!; | ||
return { | ||
...clientConfig, | ||
host: resolvedServerHostname, | ||
port: resolvedServerPort, | ||
}; | ||
} |
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 am worried that changing the default fallback logic here will cause some cases to fail to connect.
In Vite, each connect will use the host of the current page first, and then use the direct host if it fails. (https://github.com/vitejs/vite/blob/main/packages/vite/src/client/client.ts#L53-L71)
However, this PR forces all re-connects to use only the direct host, but it is obvious that it cannot be connected in some scenarios, such as on Cloud IDE.
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 have changed to onError instead of onClose so that it wont break existing reconnect logic. Would this be better?
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.
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.
Nice, using
onError
looks good to me 👍