fix server transformer not respecting host and port overrides #33425
Conversation
| const ESM = !!argv.esm; | ||
|
|
||
| const defaultTransformConfig = { | ||
| esm: ESM, | ||
| host: HOST, |
There was a problem hiding this comment.
Host is not static, it's per request. Eg, hosting on 0.0.0.0 (the now default) allows connections on 192.168.1.100 (or whatever your local IP is). So test on a phone would request http://192.168.1.100:8000, and we should respond with URLs using that requesting domain.
There was a problem hiding this comment.
i went ahead and remove the overriding host code and removed reading from global argv
| @@ -76,12 +76,12 @@ export function toExtension(url: URL, extension: string): URL { | |||
| * This is a temporary measure to allow for a relaxed parsing of our | |||
| * fixture files' src urls before they are all fixed accordingly. | |||
| */ | |||
| export function tryGetUrl(src: string, port: number = 8000): URL { | |||
| export function tryGetUrl(src: string, host: string = '0.0.0.0', port: number = 8000): URL { | |||
There was a problem hiding this comment.
Depending on the caller, this is unneeded. If you already know the absolute path, just use /path/to/script.mjs. That means the returned URL will always be on whatever host is currently being requested, without you having to know about it.
There was a problem hiding this comment.
went ahead and applied your suggestion. PTAL
There was a problem hiding this comment.
This is a separate concern. What I mean here is that the caller of tryGetUrl probably doesn't need to call it at all. Instead, they can just use an host-relative absolute path (eg, /path/to/script.js).
| @@ -55,7 +55,7 @@ export function CDNURLToLocalDistURL( | |||
| useMaxNames = false, | |||
| ): URL { | |||
| url.protocol = 'http'; | |||
| url.hostname = 'localhost'; | |||
| url.hostname = '0.0.0.0'; | |||
There was a problem hiding this comment.
@jridgewell is this correct?
There was a problem hiding this comment.
also question does anyone know if 0.0.0.0 works on windows?
/cc @rsimha
There was a problem hiding this comment.
No, this isn't right. If i request the page from my phone, 0.0.0.0 is going to tell me to look up this URL on my phone server, which doesn't exist. You need to know the host of the page is currently being requested, and rewrite the URL relative to that host.
There was a problem hiding this comment.
We run integration tests on Windows, so you can look at logs from older CI builds. This build suggests that Windows doesn't use 0.0.0.0, but it does use 127.0.0.1.
erwinmombay
force-pushed
the
fix-port-host-new-server
branch
2 times, most recently
from
b4a9f5b
to
2cf1359
Compare
Co-authored-by: Raghu Simha <rsimha@amp.dev>
Co-authored-by: Raghu Simha <rsimha@amp.dev>
erwinmombay
force-pushed
the
fix-port-host-new-server
branch
from
2cf1359
to
e0b3a8e
Compare
| @@ -77,6 +77,17 @@ export function CDNURLToLocalDistURL( | |||
| return url; | |||
| } | |||
|
|
|||
| export function CDNURLToLocalHostRelativeAbsoluteDist( | |||
There was a problem hiding this comment.
This function name is a mouthful to say, but I don't have any better suggestions 😃
currently when a port and host override is passed in from
gulp servethe transformer does not currently apply the values to the transformed document. this breaks some port forwarding scenarios as well as being able to override the host and port values manually.