Skip to content
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

support ipv6 by default #1146

Closed
wants to merge 2 commits into from
Closed

Conversation

sinasab
Copy link
Contributor

@sinasab sinasab commented Oct 9, 2024

Since updating the internal server to hono, it seems this behavior has changed; looks like ponder always defaults to using 0.0.0.0 as the hostname when unspecified, rather than adaptively choosing between that and :: as node did. See here for where this behavior lives.

I added a small fix that defaults the hostname arg to :: when unset, and a test that validates this behavior.

As with before, this can easily be worked around via using the --hostname flag (eg. ponder start --hostname=::)

@typedarray
Copy link
Collaborator

Thanks for opening and apologies for the regression.

Rather than update our default to override theirs, should we try to upstream this change into @hono/node-server? @kyscott18 has contributed there before.

https://github.com/honojs/node-server/blob/main/src/server.ts#L24

-  server.listen(options?.port ?? 3000, options.hostname ?? '0.0.0.0', () => {
+  server.listen(options?.port ?? 3000, options.hostname, () => {

The property that Ponder's hostname argument behaves just like the Node.js default seems desirable to me. Let me know what you think.

@sinasab
Copy link
Contributor Author

sinasab commented Oct 9, 2024

should we try to upstream this change

Nice, I like that idea. I'd assumed that getting the change into ponder would be significantly quicker than upstreaming it, but you inspired me to submit the upstream PR anyway-- it's here: honojs/node-server#206

The property that Ponder's hostname argument behaves just like the Node.js default seems desirable to me. Let me know what you think.

Yes definitely, when it works 😜 in light of it not working as expected though, I was thinking the tradeoff of not default-supporting ipv4-only platforms would be stomacheable, esp since most users will be running ponder on railway where private networking is recommended but is breaking opaquely out of the box.

I'm not sure how quickly the upstream PR will be addressed, but once it's landed I can bump the version to get it into ponder/core as well. In the meantime, feel free to leave this PR open or close it, whatever seems appropriate to you 🫡

sinasab added a commit to sinasab/ponder that referenced this pull request Oct 10, 2024
See context here: ponder-sh#1146

Upstream change to hono/node-server was included @ honojs/node-server#206

This PR bumps the version of hono/node-server to include the upstream change. I also added a test to ensure ipv6 works, which should help avoid future regressions as well!
@sinasab
Copy link
Contributor Author

sinasab commented Oct 10, 2024

Upstream change got merged! Closing this in favor of #1155

@sinasab sinasab closed this Oct 10, 2024
@sinasab sinasab deleted the sina/ipv6-support branch October 10, 2024 21:06
sinasab added a commit to sinasab/ponder that referenced this pull request Oct 10, 2024
See context here: ponder-sh#1146

Upstream change to hono/node-server was included @ honojs/node-server#206

This PR bumps the version of hono/node-server to include the upstream change. I also added a test to ensure ipv6 works, which should help avoid future regressions as well!
kyscott18 pushed a commit that referenced this pull request Oct 10, 2024
* chore: bumping hono/node-server to support ipv6

See context here: #1146

Upstream change to hono/node-server was included @ honojs/node-server#206

This PR bumps the version of hono/node-server to include the upstream change. I also added a test to ensure ipv6 works, which should help avoid future regressions as well!

* Update stupid-cooks-wash.md

---------

Co-authored-by: typedarray <90073088+0xOlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants