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

fix(node): listen on 0.0.0.0 if server.host is set to true #10282

Merged
merged 8 commits into from
Mar 1, 2024

Conversation

SatanshuMishra
Copy link
Contributor

Changes

Problem: In Astro's "server" output mode, setting server.host: true incorrectly binds the server to localhost, preventing connections from other devices.

Cause: The existing code always assigns localhost if the options.host value is boolean, regardless of whether it's true or false.

Proposed Fix: The code now correctly handles both string and boolean values for options.host:

  • String: If options.host is a string, its value is used directly.
  • True: Binds to 0.0.0.0.
  • False: Binds to localhost.
image

Testing

A new test was created to ensure this change behaved as expected. All tests were run and passed.

image

Docs

This change simply fixes a bug. No affect to user's behavior or doc changes are expected.

Thank you to @kevinzunigacuellar & @lilnasy for their contributions to this PR. This PR closes #10264.

- Added function to return proper value for host based on options.host's type and value.
- Modified hostOptions to take in host as an input.
- Added `server-host` test to test new function.
Copy link

changeset-bot bot commented Mar 1, 2024

🦋 Changeset detected

Latest commit: 1ad0754

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Mar 1, 2024
@lilnasy
Copy link
Contributor

lilnasy commented Mar 1, 2024

The changes look great!

There seems to be an extra .changeset folder. We have that in the root of the monorepo: https://github.com/withastro/astro/tree/main/.changeset

@SatanshuMishra
Copy link
Contributor Author

The changes look great!

There seems to be an extra .changeset folder. We have that in the root of the monorepo: https://github.com/withastro/astro/tree/main/.changeset

Oh! I realize my mistake now. Apologies! Will fix it!

- Removed Extra changeset from node directory.
- Added changeset at root level.
Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks @kevinzunigacuellar for the clear tests!

Sounds good! This change LGTM!

Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
@kevinzunigacuellar
Copy link
Member

Thanks to @lilnasy for all the guidance. Couldn't have done it without his help.

Copy link
Member

@kevinzunigacuellar kevinzunigacuellar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny comment to reuse some types. Thanks for putting together the PR @SatanshuMishra

packages/integrations/node/src/standalone.ts Outdated Show resolved Hide resolved
Update variable types to use pre-existing types.

Co-authored-by: Kevin Zuniga Cuellar <46791833+kevinzunigacuellar@users.noreply.github.com>
Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A little changeset suggestion ;)

.changeset/smooth-singers-kiss.md Outdated Show resolved Hide resolved
Improve wording for changset.

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
@florian-lefebvre florian-lefebvre changed the title Node Adapter server.host: true Issue fix(node): listen on 0.0.0.0 if server.host is set to true Mar 1, 2024
@florian-lefebvre florian-lefebvre merged commit b47dcaa into withastro:main Mar 1, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Mar 1, 2024
peng added a commit to peng/astro that referenced this pull request Mar 4, 2024
* main: (327 commits)
  [ci] format
  fix(node): listen on 0.0.0.0 if server.host is set to true (withastro#10282)
  [ci] format
  fix(dev): cosider `base` when special-casing `/_image` (withastro#10274)
  [ci] format
  update login flow to support Brave (withastro#10258)
  [ci] format
  improve link command (withastro#10257)
  Updates deprecated Node.js 16 github actions (withastro#10270)
  Fix Vitest check fail again (withastro#10266)
  [ci] format
  Adds auto completion of `astro:` events when adding or removing event listeners on `document` (withastro#10263)
  Update Vite to latest (withastro#10259)
  [ci] release (withastro#10236)
  [ci] format
  fix(i18n): localised index pages are overwritten (withastro#10250)
  fix: change strategy for route caching (withastro#10248)
  Fix TypeScript type definitions for `Code` component (withastro#10251)
  chang changeset (withastro#10253)
  Removes morph animations when setting transition:animate=none (withastro#10247)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using the true option in server.host doesn't work
4 participants