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

[feat] allow node adapter to configure listen path #2048

Merged

Conversation

matths
Copy link
Contributor

@matths matths commented Jul 31, 2021

The build and run process using the current @sveltejs/adapter-node only allows to run the final node project listening to either host/port or 0.0.0.0/port specified by an environment variable.
I would expect it to allow listening to a path instead as specified by node, check documentation https://nodejs.org/api/net.html#net_server_listen_path_backlog_callback

Use case:
When using a reverse proxy that expects the node application to listen for a path (domain socket file) instead of a host and port, this was currently not available in the node-adapter.
With this pull request I want to make an offer for this missing feature.

With this feature branch applied yo can now do SERVER_PATH=/tmp/socket node build to have your server listen to a domain socket file instead of a port.

I think this is somewhat related to the #1610 as this would be the other attempt to solve this problem. If the node-apapter would lead to a middleware instead of a whole node server, the developer would be free to have his own node server and have it listen to a domain socket file instead of host/port.

Tests
I did clone the existing smoke test for this use case.
And I used the changed adapter-node within my project:

  • in packages/adapter-node run pnpn build
  • copy files to your project cp packages/adapter-node/index.js packages/adapter-node/index.d.ts packages/adapter-node/files/* ~/myproject/src/lib/
  • do an import adapter from './src/lib/adapter-node/index.js';instead of import adapter from '@sveltejs/adapter-node'; in your projects svelte.config.js
  • run npm build in your project
  • to run your server use SERVER_PATH=/tmp/socket node build

Changeset
I tried to include a changeset, but I am not sure which files to add.

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2021

🦋 Changeset detected

Latest commit: a1cbc29

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-node Patch

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

@benmccann
Copy link
Member

@matths can you update the PR so that the Lint job passes?

@benmccann
Copy link
Member

I think this is somewhat related to the #1610 as this would be the other attempt to solve this problem. If the node-apapter would lead to a middleware instead of a whole node server, the developer would be free to have his own node server and have it listen to a domain socket file instead of host/port.

If you got #2051 merged would you no longer need this PR then? Because I think the more we can expose the underlying to allow users to configure it directly that's better vs trying to add all the options here individually

@matths
Copy link
Contributor Author

matths commented Aug 3, 2021

yes, I agree, we can skip and close this PR as I'll focus on #2051.

@matths matths closed this Aug 3, 2021
@benmccann benmccann reopened this Aug 6, 2021
@benmccann benmccann changed the title Allow node adapter to configure and use path instead of host/port [feat] allow node adapter to configure listen path Aug 9, 2021
@benmccann benmccann force-pushed the feature/adapter-node-support-env-path branch from c4bd0aa to 028b7a1 Compare August 12, 2021 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants