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

Host header fallback #40

Closed
wants to merge 3 commits into from
Closed

Conversation

kolchurinvv
Copy link
Contributor

after a recent update host_header is not defined / empty string ... for some reason

@kolchurinvv
Copy link
Contributor Author

solves #39

Copy link
Contributor

@silvio2402 silvio2402 left a comment

Choose a reason for hiding this comment

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

I notice you're providing a default value for host_header. Consider utilizing the env function's fallback parameter on line 23 to set "Host" as the fallback.
For example: const host_header = env("HOST_HEADER", "Host").toLowerCase();
This approach keeps default configurations centralized.
Thanks for your efforts on this!

@kolchurinvv
Copy link
Contributor Author

@silvio2402 thank you for the advice. makes sense. applied the change

@chevcast
Copy link

I've forked this repo and published my own NPM package. The fix for this issue is already merged into my fork and I plan to keep it updated. Anyone else having this issue is free to use it if you'd like:

bun add @catdadcode/svelte-adapter-bun -D

https://github.com/catdadcode/svelte-adapter-bun

I've also added signal event handling to the fork. Previously if you ran your Svelte app in a Docker container then killing the container would take 10+ seconds before Docker would force kill it. The resulting build/ app from this adapter now has proper process signal handling and will gracefully shutdown immediately upon receiving any of SIGTERM, SIGINT, SIGQUIT, SIGSTOP, or SIGKILL.

@silvio2402
Copy link
Contributor

I've forked this repo and published my own NPM package. The fix for this issue is already merged into my fork and I plan to keep it updated. Anyone else having this issue is free to use it if you'd like:

Thanks for your efforts. It'd also be great if you could make a PR in this repository so all users can benefit from your changes.

@chevcast
Copy link

chevcast commented Nov 25, 2023

I've forked this repo and published my own NPM package. The fix for this issue is already merged into my fork and I plan to keep it updated. Anyone else having this issue is free to use it if you'd like:

Thanks for your efforts. It'd also be great if you could make a PR in this repository so all users can benefit from your changes.

I'll be happy to do so if this repo starts rolling in changes again. As it stands Bun is linking over to my fork for now (I didn't ask for that. Jarred just noticed somehow.). I'll be rewriting this adapter a bit as well, including adding tests.

@dimdenGD
Copy link

dimdenGD commented Feb 1, 2024

@gornostay25 Why you're not merging this? Currently every single person who installs your adapter gets this error and has to look into open issues and then find this PR and get the fork instead.

@kolchurinvv
Copy link
Contributor Author

I'd just switch to the adapter maintained by @catdadcode 😃

@dimdenGD
Copy link

dimdenGD commented Feb 1, 2024

It'd probably be nice to include that in the Readme

@gornostay25
Copy link
Owner

Fixed: #47

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.

5 participants