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

Makes Node adapter respect HOST (#1365) #1366

Merged
merged 9 commits into from
May 7, 2021
Merged

Makes Node adapter respect HOST (#1365) #1366

merged 9 commits into from
May 7, 2021

Conversation

tsanth
Copy link
Contributor

@tsanth tsanth commented May 6, 2021

By default, the HOST variable is set to 127.0.0.1, for safety.

I lack the experience to set up a test runnable via pnpm test, but I've tested this as follows:

  • I add in my PR.
  • npm run build
  • HOST=192.168.1.64 npm start
  • Verify that I can reach my server only from its IPv4 address
  • Back out my PR
  • npm run build
  • HOST=192.168.1.64 npm start
  • Verify that I can reach my server from all of 1) localhost, 2) IPv4, and 3) IPv6

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

tsanth added 2 commits May 6, 2021 13:35
By default, the HOST variable is set to 127.0.0.1, for safety.
@Rich-Harris
Copy link
Member

Thank you — is there a reason to default to 127.0.0.1? Given that this is intended for production servers, surely we want the address to be unspecified?

@benmccann benmccann linked an issue May 6, 2021 that may be closed by this pull request
@tsanth
Copy link
Contributor Author

tsanth commented May 6, 2021

My typical workflow is to use a local nginx instance to reverse proxy requests to Node (to simplify SSL setup, among things), so I set the default assuming that use case.

Since this is for production servers, I've modifed the default to be 0.0.0.0 instead, so as to capture all interfaces.

@Rich-Harris
Copy link
Member

Thank you — I took the liberty of adding a changeset and docs

@@ -2,9 +2,9 @@ import { createServer } from './server';
/*eslint import/no-unresolved: [2, { ignore: ['\.\/app\.js$'] }]*/
import * as app from './app.js';

const { PORT = 3000 } = process.env; // TODO configure via svelte.config.js
const { HOST = '0.0.0.0', PORT = 3000 } = process.env; // TODO configure via svelte.config.js
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO still a TODO? Would the env vars take precedence?

Copy link
Member

Choose a reason for hiding this comment

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

it's still a TODO insofar as you can't specify a default port/host via the adapter config. Is that something worth having, if we have environment variables?

Copy link
Member

Choose a reason for hiding this comment

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

Well yeah what I meant is: is this something we still want to do? I don't have especially strong opinions about that. I assume it would be additional args you pass to the adapter factory function and not somewhere else in the config.

Copy link
Member

Choose a reason for hiding this comment

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

it would be, yeah. tbh i'm inclined to just remove the TODO comment — it's less confusing if there's only one way to set non-default values, and we need environment variables regardless so that it's configurable at runtime

@Rich-Harris Rich-Harris merged commit 0e09581 into sveltejs:master May 7, 2021
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.

Make the Node adapter respect the HOST environment variable
3 participants