feat(dev): Allow setting host and port #1816
Conversation
bastilian
force-pushed
the
1811
branch
2 times, most recently
from
2a22139
to
84a843d
Compare
tests/test_utils.ts
Outdated
| @@ -426,7 +426,8 @@ async function spawnServer( | |||
| // @ts-ignore yes it does | |||
| for await (const line of lines.values({ preventCancel: true })) { | |||
| output.push(line); | |||
| const match = line.match(/https?:\/\/localhost:\d+/g); | |||
| const match = line.match("Fresh ready"); | |||
There was a problem hiding this comment.
Since the hostname/IP can now be variable this needed to be changed to assert the fresh is running. "Fresh ready" seemed to be better as it'll always be there and not dynamically change.
| if (match) { | ||
| const running = line.match("Fresh ready"); | ||
|
|
||
| if (running) { | ||
| address = match[0]; |
There was a problem hiding this comment.
This line is likely causing the tests to fail. Previously the regex was used to grab the actual server address out of the child process stdout and that's missing now.
There was a problem hiding this comment.
I reverted the change to "Fresh ready" and just made the regex less strict, which should do the trick.
|
The test added for the |
bastilian
force-pushed
the
1811
branch
2 times, most recently
from
589808b
to
52672eb
Compare
|
Just checking up on here. If this should have more tests, it might be a better path to first add general tests for the |
|
@marvinhagemeister After playing with this more, I felt it would be better if flags or environment variables would override what is passed in the options. |
bastilian
force-pushed
the
1811
branch
2 times, most recently
from
5abca40
to
5da77f4
Compare
|
ping. Let me know if this PR needs more work. :) |
There was a problem hiding this comment.
Hi @bastilian, cool feature. A few comments:
- looks like a conflict has crept in (I think due to one of my PRs that got merged) -- please merge main and clean that up
- the rest are inline
| @@ -39,13 +39,20 @@ If you want to change the port or host, modify the config bag of the `start()` | |||
| call in `main.ts` to include an explicit port number: | |||
|
|
|||
| ```js main.ts | |||
There was a problem hiding this comment.
I know this isn't from you, but we no longer modify main.ts to do this. The correct spot is now in fresh.config.ts. (Otherwise these changes wouldn't take place when running in dev mode.) Additionally, setting port and hostname directly are deprecated. Please see FreshConfig for details.
I guess the correct thing would be something like:
export default defineConfig({
server: {
port: 3000,
hostname: "0.0.0.0",
},
});There was a problem hiding this comment.
I wasn't sure on how to properly update the docs properly. This helped! Thank you for the hint!
| console.log(` ${localLabel} ${address}\n`); | ||
| }; | ||
| if (portEnv !== undefined || portFlag !== undefined) { | ||
| opts.port = parseInt((portFlag || portEnv) as string, 10); |
There was a problem hiding this comment.
Isn't 10 the default? Do you really need to declare it?
There was a problem hiding this comment.
It's unfortunately a little more complex. Quoting from MDN:
- If the input string, with leading whitespace and possible +/- signs removed, begins with 0x or 0X (a zero, followed by lowercase or uppercase X), radix is assumed to be 16 and the rest of the string is parsed as a hexadecimal number.
- If the input string begins with any other value, the radix is 10 (decimal).
So better to always pass the radix argument when using parseInt
There was a problem hiding this comment.
Thanks for the link and the clarification!
tests/main_test.ts
Outdated
| @@ -841,6 +841,27 @@ Deno.test("PORT environment variable", async () => { | |||
| for await (const _ of lines) { /* noop */ } | |||
| }); | |||
|
|
|||
| Deno.test("HOST environment variable", async () => { | |||
There was a problem hiding this comment.
I see your comment about testing port, but since Fresh relies on Deno.serve, I guess there aren't any tests for testing hostname/port from the options bag. We can assume it works from upstream.
Perhaps you could modify this to also set the port via an environment variable?
There was a problem hiding this comment.
Add the PORT to this test. Thank you!
bastilian
force-pushed
the
1811
branch
3 times, most recently
from
d5e7811
to
ae3b4c2
Compare
There was a problem hiding this comment.
I unfortunately think my comment from here applies as well:
Now that I think about it more, this should probably be branched to a canary version.
If someone reads this prior to the next release, they could end up confused. https://github.com/denoland/fresh/pull/1984/files#diff-40ec5d53f77de7b64b64ce31562e1717e678c4c55640531b6a55d2cbc9f53936L57 provides an example of how to modify the toc.ts file to have a different canary page.
In the future I'll make sure to make this suggestion straight away when people are adding new features.
|
@deer Thank you for taking a look again! The updated docs should now be under canary. |
This adds
--hostand--portflags to allow running fresh with a custom IP and port to listen on for requests.